Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Updated single command How to #6876

Merged
merged 1 commit into from
Oct 6, 2016
Merged

Conversation

mickaelandrieu
Copy link
Contributor

@mickaelandrieu mickaelandrieu commented Aug 15, 2016

Hi,

I've updated "Single command application" tutorial, regarding the recent pull request merged (symfony/symfony#16906).

Mickaël

@lyrixx
Copy link
Member

lyrixx commented Aug 29, 2016

This PR will fix #6729

@mickaelandrieu
Copy link
Contributor Author

@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)
Copy link
Member

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 the true parameter)

Copy link
Contributor Author

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 :)

Copy link
Contributor

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
Copy link
Member

lyrixx commented Aug 29, 2016

@lyrixx would you mind to review it ?

👍 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')
Copy link
Contributor

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?

@mickaelandrieu mickaelandrieu force-pushed the patch-2 branch 2 times, most recently from 3e042a0 to a78ff44 Compare August 31, 2016 08:00

$application = new MyApplication();
$application->run();
it is possible to remove this need declaring a single command application::
Copy link
Member

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 the by in ...this need by declaring a single..., it's clearer imho

Copy link
Contributor Author

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes it is :)

Copy link
Contributor Author

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 !

@mickaelandrieu mickaelandrieu force-pushed the patch-2 branch 3 times, most recently from 49e3edd to b6d06fa Compare August 31, 2016 10:25
@mickaelandrieu
Copy link
Contributor Author

mickaelandrieu commented Aug 31, 2016

@chalasr now Travis is complaining, probably cause of my use of :method: ... any idea ? We are close to "good enough", thanks for your help !

@chalasr
Copy link
Member

chalasr commented Aug 31, 2016

Status: reviewed

👍


Of course, you can still register a command as usual::

#!/usr/bin/env php
Copy link
Member

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

Copy link
Contributor Author

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
Copy link
Member

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 the setDefaultCommand() method that this always is the case anyway).

Copy link
Member

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

Copy link
Contributor Author

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 to setDefaultCommand 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
Copy link
Member

I think this is great (both the docs and the simplification itself). Thanks Mickaël!

@weaverryan weaverryan merged commit 5535c88 into symfony:master Oct 6, 2016
weaverryan added a commit that referenced this pull request Oct 6, 2016
This PR was merged into the master branch.

Discussion
----------

Updated single command How to

Hi,

I've updated "Single command application" tutorial, regarding the recent pull request merged (symfony/symfony#16906).

Mickaël

Commits
-------

5535c88 Updated single command How to
@mickaelandrieu mickaelandrieu deleted the patch-2 branch October 6, 2016 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants