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] Add option to automatically run suggested command if there is only 1 alternative #25732

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@pierredup
Contributor

pierredup commented Jan 9, 2018

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

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Jan 9, 2018

@pierredup pierredup changed the title from [Console] Add options to automatically run suggested command if there is only 1 alternative to [Console] Add option to automatically run suggested command if there is only 1 alternative Jan 9, 2018

*/
class NamespaceNotFoundException extends CommandNotFoundException
{
}

This comment has been minimized.

@chalasr

chalasr Jan 11, 2018

Member

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 extending CommandNotFoundException, new unhandled exceptions in userland).

This comment has been minimized.

@pierredup

pierredup Jan 11, 2018

Contributor

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 from CommandNotFound seemed weird as they are for different use cases, which is why I added the deprecation

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

This comment has been minimized.

@chalasr

chalasr Jan 11, 2018

Member

@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.

@@ -226,7 +228,18 @@ public function doRun(InputInterface $input, OutputInterface $output)
$e = $event->getError();
}
throw $e;
if (!($e instanceof CommandNotFoundException && !$e instanceof NamespaceNotFoundException) || 1 !== count($alternatives = $e->getAlternatives()) || !$input->isInteractive()) {

This comment has been minimized.

@chalasr

chalasr Jan 12, 2018

Member

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)

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

This comment has been minimized.

@chalasr

chalasr Jan 12, 2018

Member

please revert, should be done on 3.4

@@ -1,5 +1,5 @@
In ApplicationTest.php line 770:
In ApplicationTest.php line 795:

This comment has been minimized.

@chalasr

chalasr Jan 12, 2018

Member

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

This comment has been minimized.

@pierredup

pierredup Jan 15, 2018

Contributor

Addressed in #25794

$question = new ConfirmationQuestion(sprintf("<error>Command \"%s\" is not defined.</error>\n\nDo you want to run \"%s\" instead? [y/n] ", $name, $alternative), false);
if (!(new QuestionHelper())->ask($input, $output, $question)) {
return 1;

This comment has been minimized.

@chalasr

chalasr Jan 12, 2018

Member

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

@chalasr

This comment has been minimized.

Member

chalasr commented Jan 12, 2018

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

@chalasr chalasr self-requested a review Jan 19, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Jan 23, 2018

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

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

This comment has been minimized.

@chalasr

chalasr Jan 29, 2018

Member

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

This comment has been minimized.

@pierredup

pierredup Jan 29, 2018

Contributor

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)

This comment has been minimized.

@pierredup

pierredup Jan 29, 2018

Contributor

Added another test case to cover namespace error

@fabpot

fabpot approved these changes Feb 7, 2018

return 1;
}
$command = $this->find($alternative);

This comment has been minimized.

@chalasr

chalasr Feb 8, 2018

Member

I suggest to use SymfonyStyle instead wrong line

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

This comment has been minimized.

@chalasr

chalasr Feb 8, 2018

Member

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

This comment has been minimized.

@chalasr

chalasr Feb 8, 2018

Member

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

$alternative = $alternatives[0];
$question = new ConfirmationQuestion(sprintf("<error>Command \"%s\" is not defined.</error>\n\nDo you want to run \"%s\" instead? [y/n] ", $name, $alternative), false);
if (!(new QuestionHelper())->ask($input, $output, $question)) {

This comment has been minimized.

@chalasr

chalasr Feb 11, 2018

Member

I suggest to use SymfonyStyle instead

pierredup added some commits Feb 11, 2018

@chalasr

This comment has been minimized.

Member

chalasr commented Feb 17, 2018

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

@pierredup

This comment has been minimized.

Contributor

pierredup commented Feb 22, 2018

New failures on travis seems unrelated

@fabpot

fabpot approved these changes Feb 22, 2018

@fabpot

This comment has been minimized.

Member

fabpot commented Feb 22, 2018

Thank you @pierredup.

@fabpot 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…
…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 pierredup deleted the pierredup:console-run-command-suggestion branch Feb 22, 2018

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment