[Console] Added the possibility to set a different default command by dcsg · Pull Request #9776 · symfony/symfony

dcsg · 2013-12-14T16:06:39Z

I am not quite sure if this is the best approach to solve the issue but the solution I provide works. Let me know your suggestions to improve it.

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#8058
License	MIT
Doc PR	symfony/symfony-docs#3426

jakzal · 2014-01-04T20:20:41Z

src/Symfony/Component/Console/Application.php

This method does two things:

sets a default command

adds a command

I'd make it only does what the method name says (set a default command) and make it accept a command name, not a whole command.

@jakzal I hadn't thought that way, but I do think it will work fine the way you say and in a more simple way. Will test it and if works well will do the changes

dcsg · 2014-01-04T20:52:57Z

@jakzal thanks for your tip. Now is more simple and works fine. Do you think is ok to be merged? (after squash)

jakzal · 2014-01-04T20:53:49Z

src/Symfony/Component/Console/Application.php

No longer adds ;)

true 👍 already modified and squashed!

dcsg · 2014-01-04T20:59:20Z

Also I do need to create an entry in documentation for the feature. Will take care of that.

* Added new method `setDefaultCommand` and changed the logic in the doRun method in order to allow it to run by default a Command setted by the user instead of the `ListCommand` * Added tests * Should fix symfony#8058 * Updated CHANGELOG

dcsg · 2014-01-07T10:15:12Z

@jakzal @fabpot is the implementation ok? Docs are already created as well.

piotrpasich · 2014-01-07T10:18:33Z

Looks ok for me 👍
On the other hand, is it possible to create some configuration option in config.yml for this feature?

dcsg · 2014-01-07T10:26:17Z

@piotrpasich that is Framework related. Also I do think for the framework itself could not make so much sense to do that, but if it does IMO it should be a different PR

piotrpasich · 2014-01-07T10:29:09Z

@danielcsgomes I'm not sure if this should be in second PR, but ok. We can open a new issue to remember about this change.

fabpot · 2014-01-07T10:30:31Z

@piotrpasich I'm -1 for adding an option in config.yml. To me, the possibility to change the default command is very specific and probably never a good idea in the full-stack framework itself.

fabpot · 2014-01-07T10:30:58Z

src/Symfony/Component/Console/Tests/ApplicationTest.php

we don't put spaces around . in Symfony CS.

stof · 2014-01-07T10:44:15Z

I see an issue here (something we faced in PhpSpec, where we finally chose to remove the overwrite of the default command for now): the default command cannot get arguments (the first one would be interpreted as a command name), so changing the default command will only be reliable if the custom command does not have arguments

fabpot · 2014-01-07T12:49:58Z

@stof That's to be expected as we are talking about a "default" command to be executed when no arguments are passed to the console application. So, this PR is different from #9564, where you might want to create a Console app with only 1 command.

stof · 2014-01-07T12:53:48Z

yeah, I'm just warning that it might cause some issues if pople start expecting the command to work fully when using it as the default one

wouterj · 2014-01-07T12:55:17Z

Well, I think @danielcsgomes did a good work at the documentation for this PR. The article clearly has 2 sections: setting the default and creating a one command app. Both explain other ways to do it, so I don't think people will get confused.

fabpot · 2014-01-07T13:17:01Z

src/Symfony/Component/Console/Tests/ApplicationTest.php

set instead of setted

dcsg · 2014-01-07T14:48:42Z

@stof @fabpot yes that is a limitation. I will add that information to the Documentation.

fabpot · 2014-01-07T15:05:08Z

src/Symfony/Component/Console/Application.php

missing dot at the end.

fabpot · 2014-01-07T15:22:37Z

It took time, but here we go, this is in now. Thank you very much @danielcsgomes.

dcsg · 2014-01-07T15:25:19Z

@fabpot my pleasure :)

@wouterj

…component (danielcsgomes) This PR was merged into the master branch. Discussion ---------- New Feature: Change the Default Command in the Console component | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#9776) | Applies to | 2.5+ | Fixed tickets | I followed the suggestion from @wouterj in IRC on changing the `single_command_tool` and added the documentation for the new feature there. Commits ------- c1b2aad Applied suggestions from Ryan 5e97202 Applyed suggestions from @fabpot and @stof c23f34e Applied some suggestions 012456d Moved `versionadded` to the right section 730985f Updated references to the new document af9eac4 Changed the code to remove references to Symfony Framework since it's the standalone component 60e2b3e Added the delete document to avoid broken urls and added a notice that the document was moved to another location 11c7174 Added the version number where the setDefaultCommand was introduced b29ab89 Documented the Change the Default Command in the Console component

jakzal reviewed Jan 4, 2014
View reviewed changes

dcsg mentioned this pull request Jan 4, 2014

New Feature: Change the Default Command in the Console component symfony/symfony-docs#3426

Merged

fabpot reviewed Jan 7, 2014
View reviewed changes

Removed white spaces

d53bd86

fabpot reviewed Jan 7, 2014
View reviewed changes

src/Symfony/Component/Console/Tests/ApplicationTest.php Outdated

Copy link
Copy Markdown

Member

fabpot Jan 7, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

set instead of setted

Applyed @fabpot suggestions

e2da1c3

fabpot reviewed Jan 7, 2014
View reviewed changes

src/Symfony/Component/Console/Application.php Outdated

Copy link
Copy Markdown

Member

fabpot Jan 7, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing dot at the end.

Added dot at the end of sentence

c070b80

fabpot closed this in c833518 Jan 7, 2014

dcsg deleted the ticket_8058 branch January 7, 2014 15:25

jakzal mentioned this pull request Jan 10, 2014

It should return non 0 exit code when suite is empty phpspec/phpspec#238

Open

Uh oh!

Conversation

dcsg commented Dec 14, 2013

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcsg commented Jan 4, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcsg commented Jan 4, 2014

Uh oh!

dcsg commented Jan 7, 2014

Uh oh!

piotrpasich commented Jan 7, 2014

Uh oh!

dcsg commented Jan 7, 2014

Uh oh!

piotrpasich commented Jan 7, 2014

Uh oh!

fabpot commented Jan 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stof commented Jan 7, 2014

Uh oh!

fabpot commented Jan 7, 2014

Uh oh!

stof commented Jan 7, 2014

Uh oh!

wouterj commented Jan 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dcsg commented Jan 7, 2014

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fabpot commented Jan 7, 2014

Uh oh!

dcsg commented Jan 7, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants