NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Console] Add option to automatically run suggested command if there is only 1 alternative#25732

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

Jump to bottom

Closed

pierredup wants to merge9 commits intosymfony:masterfrompierredup:console-run-command-suggestion

Closed

[Console] Add option to automatically run suggested command if there is only 1 alternative#25732

pierredup wants to merge9 commits intosymfony:masterfrompierredup:console-run-command-suggestion

Conversation

Copy link

Contributor

pierredup commentedJan 9, 2018•
edited
Loading

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	yes
Tests pass?	yes
Fixed tickets
License	MIT
Doc PR

When mistyping a console command, you get an error giving suggested commands.
If there is only 1 alternative suggestion, this PR will give you the option to run that command instead. This makes it easier to run the correct command without having to re-type/copy-paste/update the previous run command

carsonbot added Status: Needs Review Console Feature Deprecation labels

Jan 9, 2018

pierredup force-pushed theconsole-run-command-suggestion branch frome8f9bdf to5abc30fCompare

January 9, 2018 14:02

nicolas-grekas added this to the4.1 milestone

Jan 9, 2018

pierredup changed the title~~[Console] Add options to automatically run suggested command if there is only 1 alternative~~[Console] Add option to automatically run suggested command if there is only 1 alternative

Jan 9, 2018

pierredup force-pushed theconsole-run-command-suggestion branch 2 times, most recently from3524d9d tobc36d20Compare

January 9, 2018 20:26

chalasr reviewed

Jan 11, 2018

View reviewed changes

src/Symfony/Component/Console/Exception/NamespaceNotFoundException.php

		*/
		class NamespaceNotFoundExceptionextends CommandNotFoundException
		{
		}

Copy link

Member

chalasrJan 11, 2018•
edited
Loading

Choose a reason for hiding this comment

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

This addition is not strictly needed, right? Not sure it's useful, and it involves a BC break with no upgrade path in the future (stop extendingCommandNotFoundException, new unhandled exceptions in userland).

Copy link

ContributorAuthor

pierredupJan 11, 2018

Choose a reason for hiding this comment

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

No it's not strictly needed, but I needed a way to distinguish between not finding a namespace and not finding a command to prevent incorrect suggestions when typing an incorrect namespace. Keeping this class extending fromCommandNotFound seemed weird as they are for different use cases, which is why I added the deprecation

src/Symfony/Component/Console/Exception/NamespaceNotFoundException.php Outdated

		*
		* @author Pierre du Plessis <pdples@gmail.com>
		*
		* @deprecated This class won't extend CommandNotFoundException in Symfony 5

Copy link

Member

chalasrJan 11, 2018

Choose a reason for hiding this comment

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

@deprecated should only be used when a class is actually deprecated, the debug class loader parses such annotations, could lead to wrong notices e.g. when extending it.

chalasr requested changes

Jan 12, 2018

View reviewed changes

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

		}

		throw$e;
		if (!($einstanceof CommandNotFoundException && !$einstanceof NamespaceNotFoundException) \|\|1 !==count($alternatives =$e->getAlternatives()) \|\| !$input->isInteractive()) {

Copy link

Member

chalasrJan 12, 2018

Choose a reason for hiding this comment

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

Doing this here keeps dispatching a console.error event, not sure it's desired in case the user choose to run the single alternative? Especially given it's currently not taking into account the potential changes made on the event (see comment below)

src/Symfony/Component/Console/CHANGELOG.md Outdated

		* removed`ConsoleExceptionEvent`
		* removed`ConsoleEvents::EXCEPTION`
		* removed`ConsoleExceptionEvent`
		* removed`ConsoleEvents::EXCEPTION`

Copy link

Member

chalasrJan 12, 2018

Choose a reason for hiding this comment

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

please revert, should be done on 3.4

src/Symfony/Component/Console/Tests/Fixtures/application_renderexception_linebreaks.txt Outdated

		@@ -1,5 +1,5 @@

		In ApplicationTest.php line770:
		In ApplicationTest.php line795:

Copy link

Member

chalasrJan 12, 2018

Choose a reason for hiding this comment

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

Going to be a nightmare, we should make the related tests more flexible by using a placeholder where they have been introduced

Copy link

ContributorAuthor

pierredupJan 15, 2018

Choose a reason for hiding this comment

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

Addressed in#25794

src/Symfony/Component/Console/Application.php

		$question =newConfirmationQuestion(sprintf("<error>Command\"%s\" is not defined.</error>\n\nDo you want to run\"%s\" instead? [y/n]",$name,$alternative),false);

		if (!(newQuestionHelper())->ask($input,$output,$question)) {
		return1;

Copy link

Member

chalasrJan 12, 2018

Choose a reason for hiding this comment

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

The error event allows to set a custom exit code and override the exception, it must keep working

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Jan 12, 2018

Copy link

Member

chalasr commentedJan 12, 2018•
edited
Loading

No it's not strictly needed, but I needed a way to distinguish between not finding a namespace and not finding a command to prevent incorrect suggestions when typing an incorrect namespace

How that? AFAIU it behaves the same for both exceptions currently

pierredup force-pushed theconsole-run-command-suggestion branch from6c1cd01 to34222d7Compare

January 15, 2018 12:31

chalasr self-requested a review

January 19, 2018 14:47

carsonbot added Status: Needs Review and removed Status: Needs Work labels

Jan 19, 2018

Copy link

Member

fabpot commentedJan 23, 2018

@pierredup Can you rebase to get rid of the merge commit? We never merge a PR with a merge commit. Thanks.

pierredup added3 commits

January 23, 2018 08:33

Add options to automatically run suggested command if there is only 1…

5bdcdda

… alternative

Remove deprecation and future BC break

d93ca23

Revert unrelated changes in the changelog

f42a2bb

pierredup force-pushed theconsole-run-command-suggestion branch from6e93303 to222af7cCompare

January 23, 2018 06:36

Dispatch console error event

e4deda7

pierredup force-pushed theconsole-run-command-suggestion branch from222af7c toe4deda7Compare

January 23, 2018 06:38

chalasr reviewed

Jan 29, 2018

View reviewed changes

src/Symfony/Component/Console/Application.php

		if (null !==$this->dispatcher) {
		$event =newConsoleErrorEvent($input,$output,$e);
		$this->dispatcher->dispatch(ConsoleEvents::ERROR,$event);
		if (!($einstanceof CommandNotFoundException && !$einstanceof NamespaceNotFoundException) \|\|1 !==count($alternatives =$e->getAlternatives()) \|\| !$input->isInteractive()) {

Copy link

Member

chalasrJan 29, 2018

Choose a reason for hiding this comment

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

This proposes to run the alternative even if it's a namespace only, right? A test case would be good to make sure it does not

Copy link

ContributorAuthor

pierredupJan 29, 2018

Choose a reason for hiding this comment

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

No, if it's a namespace only then the behaviour stays unchanged (Which would giving you an error and giving a list suggested namespaces without the option to run a command)

Copy link

ContributorAuthor

pierredupJan 29, 2018

Choose a reason for hiding this comment

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

Added another test case to cover namespace error

Dont run command if suggestion is only a namespace

1b9f9eb

fabpot approved these changes

Feb 7, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Review labels

Feb 7, 2018

chalasr reviewed

Feb 8, 2018

View reviewed changes

src/Symfony/Component/Console/Application.php

		return1;
		}

		$command =$this->find($alternative);

Copy link

Member

chalasrFeb 8, 2018•
edited
Loading

Choose a reason for hiding this comment

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

~~I suggest to useSymfonyStyle instead~~ wrong line

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

		$this->assertRegExp('/There are no commands defined in the "foo2" namespace./',$e->getMessage(),'->find() throws aCommandNotFoundException if namespace does not exist, with alternative');
		$this->assertRegExp('/foo/',$e->getMessage(),'->find() throws aCommandNotFoundException if namespace does not exist, with alternative : "foo"');
		$this->assertRegExp('/foo1/',$e->getMessage(),'->find() throws aCommandNotFoundException if namespace does not exist, with alternative : "foo1"');
		$this->assertRegExp('/foo3/',$e->getMessage(),'->find() throws aCommandNotFoundException if namespace does not exist, with alternative : "foo3"');

Copy link

Member

chalasrFeb 8, 2018

Choose a reason for hiding this comment

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

They are still valid and ensure the new class keeps extending it (prevent a BC break)

Copy link

Member

chalasrFeb 8, 2018

Choose a reason for hiding this comment

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

I would revert and only add a new assertion, avoiding merge conflicts

chalasr reviewed

Feb 11, 2018

View reviewed changes

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

		$alternative =$alternatives[0];
		$question =newConfirmationQuestion(sprintf("<error>Command\"%s\" is not defined.</error>\n\nDo you want to run\"%s\" instead? [y/n]",$name,$alternative),false);

		if (!(newQuestionHelper())->ask($input,$output,$question)) {

Copy link

Member

chalasrFeb 11, 2018

Choose a reason for hiding this comment

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

I suggest to useSymfonyStyle instead

pierredup added3 commits

February 11, 2018 20:49

Use SymfonyStyle

f8a816d

Revert some changes to tests

bf46ffe

Fix CS

d7dc6c7

Copy link

Member

chalasr commentedFeb 17, 2018

@pierredup Can you look at the failing tests? looks like a spacing issue.

Change spacing

4de52d2

Copy link

ContributorAuthor

pierredup commentedFeb 22, 2018

New failures on travis seems unrelated

fabpot approved these changes

Feb 22, 2018

View reviewed changes

Copy link

Member

fabpot commentedFeb 22, 2018

Thank you@pierredup.

fabpot closed this

Feb 22, 2018

fabpot added a commit that referenced this pull request

Feb 22, 2018

feature#25732[Console] Add option to automatically run suggested co…

7b8934b

…mmand if there is only 1 alternative (pierredup)This PR was squashed before being merged into the 4.1-dev branch (closes#25732).Discussion----------[Console] Add option to automatically run suggested command if there is only 1 alternative| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | yes| Tests pass?   | yes| Fixed tickets || License       | MIT| Doc PR        |When mistyping a console command, you get an error giving suggested commands.If there is only 1 alternative suggestion, this PR will give you the option to run that command instead. This makes it easier to run the correct command without having to re-type/copy-paste/update the previous run command![console](https://user-images.githubusercontent.com/144858/34724377-4b46c726-f556-11e7-94a3-a9d7c9d75e74.gif)Commits-------83d52f0 [Console] Add option to automatically run suggested command if there is only 1 alternative

pierredup deleted the console-run-command-suggestion branch

February 22, 2018 06:50