Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Console] Better support for one command app#16906

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

Merged
fabpot merged 1 commit intosymfony:masterfromlyrixx:console-one-app
Jun 15, 2016

Conversation

@lyrixx
Copy link
Member

@lyrixxlyrixx commentedDec 8, 2015•
edited
Loading

QA
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#9564
LicenseMIT

Hello;

I write many CLI application, and "single command" in cli application is not so easy to write.
This is why I propose this patch. IMHO, this PR could replaces#9609.

See it in application:

#!/usr/bin/env php<?phprequire__DIR__.'/vendor/autoload.php';useSymfony\Component\Console\Application;useSymfony\Component\Console\Input\InputArgument;useSymfony\Component\Console\Input\InputInterface;useSymfony\Component\Console\Input\InputOption;useSymfony\Component\Console\Output\OutputInterface;(newApplication('echo','1.0.0'))    ->register('echo')        ->addArgument('foo', InputArgument::OPTIONAL,'The directory','foo')        ->addOption('bar',null, InputOption::VALUE_REQUIRED,'Foobar','bar')        ->setCode(function(InputInterface$input,OutputInterface$output) {$output->writeln('start');$output->writeln($input->getArgument('foo'));$output->writeln($input->getOption('bar'));        })    ->getApplication()    ->setDefaultCommand('echo',true)    ->run();

Some usage:

>(3)[{..}eg/dev/github.com/symfony/symfony](console-one-app) php test.php startfoobar
>(3)[{..}eg/dev/github.com/symfony/symfony](console-one-app) php test.php  "first argument"startfirst argumentbar
>(3)[{..}eg/dev/github.com/symfony/symfony](console-one-app) php test.php  "first argument" --bar="first option"startfirst argumentfirst option
>(3)[{..}eg/dev/github.com/symfony/symfony](console-one-app) php test.php  "first argument" --bar="first option" --helpUsage:  echo [options] [--] [<foo>]Arguments:  foo                   The directory [default: "foo"]Options:      --bar=BAR         Foobar [default: "bar"]  -h, --help            Display this help message  -q, --quiet           Do not output any message  -V, --version         Display this application version      --ansi            Force ANSI output      --no-ansi         Disable ANSI output  -n, --no-interaction  Do not ask any interactive question  -v|vv|vvv, --verbose  Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug

@jakzal
Copy link
Contributor

"Unique command" doesn't imply that there's only one command. How about calling it a "single command"?

@lyrixx
Copy link
MemberAuthor

Indeed ;) I will update my PR.

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 also validate that it is registered ?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Triggering an exception if you configuresetUniqueCommand('foobar') and there is nofoobar command registered

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Why not, but if we do that, we are forcing the order. I mean will not able anymore to first set-up the single command name, then register the command.

@lyrixx
Copy link
MemberAuthor

PR updated ;)

@javiereguiluz
Copy link
Member

I like this proposal. However, I don't like these two lines from the above example:

    ->getApplication()    ->setSingleCommand('echo')

Would this other code work?

(newApplication())    ->register('echo')        ->addArgument('foo', InputArgument::OPTIONAL,'The directory','foo')        ->addOption('bar',null, InputOption::VALUE_REQUIRED,'Foobar','bar')        ->setCode(function(InputInterface$input,OutputInterface$output) {// ...        })    ->run();

@lyrixx
Copy link
MemberAuthor

@javiereguiluz I think we can implement that, but it's a bit too magic for me. And we don't want magic in symfony.

@lyrixx
Copy link
MemberAuthor

@javiereguiluz

What about:

(new Application('echo', '1.0.0'))    ->registerSingleCommand('echo') // <--------------------- The change is HERE        ->addArgument('foo', InputArgument::OPTIONAL, 'The directory', 'foo')        ->addOption('bar', null, InputOption::VALUE_REQUIRED, 'Foobar', 'bar')        ->setCode(function(InputInterface $input, OutputInterface $output) {            $output->writeln('start');            $output->writeln($input->getArgument('foo'));            $output->writeln($input->getOption('bar'));        })    ->run();

@nicolas-grekas
Copy link
Member

I like it :)

@javiereguiluz
Copy link
Member

I'm afraid that the new example doesn't follow the Symfony philosophy either. It complicates the learning curve because it adds a new method (registerSingleCommand()) and it doesn't follow Fabien's idea of priming composition (generic methods that can be combined, instead of specific and "niche" methods).

@lyrixx
Copy link
MemberAuthor

ThensetSingleCommand is the best choice for now.

@javiereguiluz
Copy link
Member

Yes. Would this work?

(newApplication())    ->register('echo')        ->addArgument('foo', InputArgument::OPTIONAL,'The directory','foo')        ->addOption('bar',null, InputOption::VALUE_REQUIRED,'Foobar','bar')        ->setCode(function(InputInterface$input,OutputInterface$output) {// ...        })    ->run('echo');// <-- pass the command to run

@lyrixx
Copy link
MemberAuthor

This is a good idea, but a BC break (ref)

@nicolas-grekas
Copy link
Member

->runSingleCommand('echo')?

@jvasseur
Copy link
Contributor

Why not

(newSingleCommandApplication())    ->addArgument('foo', InputArgument::OPTIONAL,'The directory','foo')    ->addOption('bar',null, InputOption::VALUE_REQUIRED,'Foobar','bar')    ->setCode(function(InputInterface$input,OutputInterface$output) {// ...    })    ->run();

?

@stof
Copy link
Member

@nicolas-grekas this might complicate the Application a lot though, asrun() is only indirectly responsible for resolving the command name, so we would have to do weird things.

@jvasseur this is a bad idea, as your SingleCommandApplication does not have the API of the Application anymore, but of the Command

@jakzal
Copy link
Contributor

@jvasseur I personally like your idea the most.

@lyrixx is it possible to achieve this with a small amount of code? :)

@lyrixx
Copy link
MemberAuthor

Yes, but whit black magic :-)

publicfunctionsetSingleCommand($commandName)
{
if (null !==$this->singleCommand) {
thrownew \LogicException('A Single command is already defined.');

Choose a reason for hiding this comment

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

I don't think this check is needed. It looks like a useless restriction to me.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

why not ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

I thinks it's useful restriction, yet it makes this feature less flexible.
And on framework level, I think it should be that way.

Strict code should be in app/domain level

@javiereguiluzjaviereguiluz changed the title[Command] Better support for one command app[Console] Better support for one command appMar 3, 2016
@@ -1,28 +1,26 @@
Usage:
help [options] [--] [<command_name>]
list [options] [--] [<namespace>]
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I updated the fixture, because now, the--help display the help of the default command.

*
* @param string $commandName The Command name
*/
publicfunctionsetDefaultCommand($commandName)
Copy link
Member

Choose a reason for hiding this comment

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

What about adding a new argument here that says it is the default but also the only command?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

And so replace the method I added? It sounds good to me.

Copy link
Member

Choose a reason for hiding this comment

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

exactly

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Done. I also added a test.

*
* @param string $commandName The Command name
* @param string $commandName The Command name
* @param bool $singleCommand Set to true if there is only one command is this Application
Copy link
Member

Choose a reason for hiding this comment

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

typo:in this

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

fixed

@fabpot
Copy link
Member

👍

* @param bool $singleCommand Set to true if there is only one command is this Application
*/
publicfunctionsetDefaultCommand($commandName)
publicfunctionsetDefaultCommand($commandName,$singleCommand =false)

Choose a reason for hiding this comment

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

Should this boolean argument be called$isSingleCommand instead?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I don't know.... Just tell me ;)

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

In my opinion it should be called$isSingleCommand because$singleCommand looks like it's asking for the name of that single command.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Fixed.

@lyrixxlyrixxforce-pushed theconsole-one-app branch 2 times, most recently froma7d0fa2 to02914e6CompareJune 15, 2016 16:05
@fabpot
Copy link
Member

Thank you@lyrixx.

@fabpotfabpot merged commit4a9bb1d intosymfony:masterJun 15, 2016
fabpot added a commit that referenced this pull requestJun 15, 2016
This PR was merged into the 3.2-dev branch.Discussion----------[Console] Better support for one command app| Q             | A| ------------- | ---| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#9564| License       | MITHello;I write many CLI application, and "single command" in cli application is not so easy to write.This is why I propose this patch. IMHO, this PR could replaces#9609.See it in application:```php#!/usr/bin/env php<?phprequire __DIR__.'/vendor/autoload.php';use Symfony\Component\Console\Application;use Symfony\Component\Console\Input\InputArgument;use Symfony\Component\Console\Input\InputInterface;use Symfony\Component\Console\Input\InputOption;use Symfony\Component\Console\Output\OutputInterface;(new Application('echo', '1.0.0'))    ->register('echo')        ->addArgument('foo', InputArgument::OPTIONAL, 'The directory', 'foo')        ->addOption('bar', null, InputOption::VALUE_REQUIRED, 'Foobar', 'bar')        ->setCode(function(InputInterface $input, OutputInterface $output) {            $output->writeln('start');            $output->writeln($input->getArgument('foo'));            $output->writeln($input->getOption('bar'));        })    ->getApplication()    ->setSingleCommand('echo')    ->run();```Some usage:```>(3)[{..}eg/dev/github.com/symfony/symfony](console-one-app) php test.phpstartfoobar``````>(3)[{..}eg/dev/github.com/symfony/symfony](console-one-app) php test.php  "first argument"startfirst argumentbar``````>(3)[{..}eg/dev/github.com/symfony/symfony](console-one-app) php test.php  "first argument" --bar="first option"startfirst argumentfirst option``````>(3)[{..}eg/dev/github.com/symfony/symfony](console-one-app) php test.php  "first argument" --bar="first option" --helpUsage:  echo [options] [--] [<foo>]Arguments:  foo                   The directory [default: "foo"]Options:      --bar=BAR         Foobar [default: "bar"]  -h, --help            Display this help message  -q, --quiet           Do not output any message  -V, --version         Display this application version      --ansi            Force ANSI output      --no-ansi         Disable ANSI output  -n, --no-interaction  Do not ask any interactive question  -v|vv|vvv, --verbose  Increase the verbosity of messages: 1 for normal output, 2 for more verbose output and 3 for debug```Commits-------4a9bb1d [Console] Better support for one command app
@lyrixxlyrixx deleted the console-one-app branchJune 15, 2016 16:21
@chalasr
Copy link
Member

Does the documentation have been updated, or it is planned to? Otherwise it should. 👍

weaverryan added a commit to symfony/symfony-docs that referenced this pull requestOct 6, 2016
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
@fabpotfabpot mentioned this pull requestOct 27, 2016
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@lyrixx@jakzal@javiereguiluz@nicolas-grekas@jvasseur@stof@fabpot@chalasr@TomasVotruba@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp