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

[Console] Added a cookbook entry on invoking other commands #4493

Closed
wants to merge 1 commit into from

Conversation

kix
Copy link
Contributor

@kix kix commented Nov 19, 2014

This is a follow-up to an issue I've opened in symfony/symfony. It describes how to find and execute other commands from a Symfony Command.


.. caution::

Note that all these commands will run in the same process, and some of Symfony's
Copy link
Member

Choose a reason for hiding this comment

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

I like this caution but I find the phrase too long to be easily understood. Could we split it into two sentences?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@javiereguiluz, looks better?

Copy link
Member

Choose a reason for hiding this comment

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

Indeed it looks better to me. Thanks!

@javiereguiluz
Copy link
Member

@kix thanks for your PR! I've found myself lost several times in the past with this issue, so this will be definitely something useful for lots of users!

@kix kix force-pushed the add-dependent-commands-cookbook branch 3 times, most recently from ffdc362 to 1d516ac Compare November 19, 2014 11:18
@kix
Copy link
Contributor Author

kix commented Nov 19, 2014

@javiereguiluz, the build passed, but my RST is a bit rusty. Does the PR look OK?

@@ -146,6 +146,40 @@ before translating contents::
However for other services the solution might be more complex. For more details,
see :doc:`/cookbook/service_container/scopes`.

Invoking other commands
Copy link
Member

Choose a reason for hiding this comment

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

Symfony docs use a special title-casing. Even if I don't fully understand it, I think that this title should be:

Invoking Other Commands

@javiereguiluz
Copy link
Member

@kix if Travis showed no error, then the RST syntax is OK. To me this PR looks correct :)

@kix kix force-pushed the add-dependent-commands-cookbook branch from 1d516ac to f6563e6 Compare November 19, 2014 11:47
@xabbuh
Copy link
Member

xabbuh commented Nov 19, 2014

We already show how to call an existing command in the components book. So I'm not sure if we really need another section for it.

@wouterj
Copy link
Member

wouterj commented Nov 23, 2014

I don't like the duplication too, but I also agree with @javiereguiluz that the caution is pretty great. Could you please add that caution to the existing section?

Also, because you proposed a new section, you probably couldn't find the already existing section. Do you have any hints/tips how to improve this, so other people will see these usefull docs?

@kix
Copy link
Contributor Author

kix commented Nov 24, 2014

@wouterj, well, here's how I didn't find it: I looked in the cookbook, and there was the section titled How to Create a Console Command, so mostly I was expecting to find everything right there. It looks like a complete guide, but the link to the console component docs was something I didn't really take into account. So yes, it might be a bit more noticeable.
On moving the caution etc.: OK, in progress.
And thanks for your comments!

@xabbuh
Copy link
Member

xabbuh commented Mar 8, 2015

@kix Can you move the caution and also replace the new example with just a reference to the already existing code?

@javiereguiluz
Copy link
Member

I've finished this PR in #5425.

@kix don't worry about not finishing this PR. This is something that happens sometimes. I've picked up your work and finished it in another PR. The new PR contains your commits, so the atribution of your work won't be lost. Thanks!

@xabbuh
Copy link
Member

xabbuh commented Jun 22, 2015

Thank you @kix for starting the work. I close here in favor of #5425.

@xabbuh xabbuh closed this Jun 22, 2015
@kix
Copy link
Contributor Author

kix commented Jun 22, 2015

@xabbuh, @javiereguiluz, sorry I didn't have time to finish this, and thanks a lot :)

@kix kix deleted the add-dependent-commands-cookbook branch June 22, 2015 15:53
weaverryan added a commit that referenced this pull request Jul 7, 2015
…javiereguiluz)

This PR was submitted for the 2.7 branch but it was merged into the 2.3 branch instead (closes #5425).

Discussion
----------

Added a caution note about invoking other commands

| Q             | A
| ------------- | ---
| Doc fix?      | no
| New docs?     | yes
| Applies to    | all
| Fixed tickets | -

This PR finishes #4493. That's why is committed against `master` branch, but it should be merged in 2.3+.

Commits
-------

2c491be Fixed typos
b86ffb6 Removed duplication and moved a caution message
57938a5 [Console] Added a cookbook entry on invoking other commands
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.

4 participants