Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork5.3k
Updated single command How to#6876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
lyrixx commentedAug 29, 2016
This PR will fix#6729 |
mickaelandrieu commentedAug 29, 2016
@lyrixx would you mind to review it ? You're the best as it's your feature, thank you. |
| $output->writeln($input->getOption('bar')); | ||
| }) | ||
| ->getApplication() | ||
| ->setDefaultCommand('echo', true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would highlight this line ; because this line is doing all the job. (especially thetrue parameter)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You're right. How about adding a comment like:
setDefaultCommand function now accepts a second argument. If true, the application
is considerated as a Single command application.
ping@wouterj as my english is just so bad :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
You could also add an inline comment like:
->setDefaultCommand('echo',true)// This line is responsible for// a single command application
lyrixx commentedAug 29, 2016
👍 But I'm not good with the documentation ;) Anyway, thanks for the doc. |
| (new Application('echo', '1.0.0')) | ||
| ->register('echo') | ||
| ->addArgument('foo', InputArgument::OPTIONAL, 'The directory', 'foo') | ||
| ->addOption('bar', null, InputOption::VALUE_REQUIRED, 'Foobar', 'bar') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What about "Foobar" => "A foobar option" to explicit the description purpose?
3e042a0 toa78ff44Compare| $application = new MyApplication(); | ||
| $application->run(); | ||
| it is possible to remove this need declaring a single command application:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Even if correct, I would let theby in...this need by declaring a single..., it's clearer imho
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
it's clearer for me too, but is it correct english ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes it is :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I go for it then :) thanks !
49e3edd tob6d06faComparemickaelandrieu commentedAug 31, 2016 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
|
chalasr commentedAug 31, 2016
Status: reviewed 👍 |
| Of course, you can still register a command as usual:: | ||
| #!/usr/bin/env php |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
we usually use four spaces to indent code blocks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Ok, I will update it :)
| ->run(); | ||
| The method:method:`Symfony\\Component\\Console\\Application::setDefaultCommand` | ||
| accepts a boolean as second parameter. If true, the command ``echo`` will then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Should we elaborate a bit on this second argument? I guess that it may sound strange that you have to pass this option at all (one might expect from the name of thesetDefaultCommand() method that this always is the case anyway).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍 for explaining what is different when passing it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not sure what you expect as an explanation, explain more is something like explain the code produced. This sounds like PHPDoc for getters/setters.
To me, in this example, the only thing to know is that passing a second parameter tosetDefaultCommand transform your application into a single command application.
If someone really want to know how we do it, I'm pretty sure he/she review the code produced (as I did ^^) ... what do you think@lyrixx ? What is really relevant to understand in your contribution ?
weaverryan commentedOct 6, 2016
I think this is great (both the docs and the simplification itself). Thanks Mickaël! |
This PR was merged into the master branch.Discussion----------Updated single command How toHi,I've updated "Single command application" tutorial, regarding the recent pull request merged (symfony/symfony#16906).MickaëlCommits-------5535c88 Updated single command How to
Uh oh!
There was an error while loading.Please reload this page.
Hi,
I've updated "Single command application" tutorial, regarding the recent pull request merged (symfony/symfony#16906).
Mickaël