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] Add completion values to input definition #44948

Merged
merged 1 commit into from Mar 17, 2022

Conversation

GromNaN
Copy link
Member

@GromNaN GromNaN commented Jan 7, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets n/a
License MIT
Doc PR symfony/symfony-docs#...

During implementation of bash completion to core commands, I found the code quite verbose. The completion for all options and arguments are mixed into the same method.

Example of current code

public function complete(CompletionInput $input, CompletionSuggestions $suggestions): void
{
if ($input->mustSuggestArgumentValuesFor('command_name')) {
$descriptor = new ApplicationDescription($this->getApplication());
$suggestions->suggestValues(array_keys($descriptor->getCommands()));
return;
}
if ($input->mustSuggestOptionValuesFor('format')) {
$helper = new DescriptorHelper();
$suggestions->suggestValues($helper->getFormats());
}
}

This PR adds the possibility to attach values to each option and argument in their definition. It provides a default implementation of Command::complete that uses this values.

In the constructor of the InputOption and InputArgument classes:

new InputArgument('command_name', InputArgument::OPTIONAL, 'The command name', 'help', function () {
return array_keys((new ApplicationDescription($this->getApplication()))->getCommands());
}),
new InputOption('format', null, InputOption::VALUE_REQUIRED, 'The output format (txt, xml, json, or md)', 'txt', function () {
return (new DescriptorHelper())->getFormats();
}),

Or using Command::addOption and Command::addArgument:

->addArgument('shell', InputArgument::OPTIONAL, 'The shell type (e.g. "bash"), the value of the "$SHELL" env var will be used if this is not given', null, fn () => $this->getSupportedShells())

Additional benefits:

Todo:

  • Add tests
  • Add info to descriptor

@GromNaN GromNaN force-pushed the console-completion-definition branch 2 times, most recently from 8650876 to dfb4cc6 Compare January 7, 2022 22:29
@wouterj
Copy link
Member

wouterj commented Jan 7, 2022

I haven't looked at the code yet, but I really like the idea. See also these 2 comments on the original PR: #42251 Being able to predict whether an option has a value or not and maybe even dumping the static values to the completion file (e.g. for apps like Composer) are 2 other very interesting advantages of this approach.

From https://github.com/posener/complete , mentioned in the comments I referenced above, I especially like the Predict enum:

 			"name":      predict.Set{"foo", "bar", "foo bar"},
 			"something": predict.Something,
 			"nothing":   predict.Nothing,

Set: values are static, can be dumped (or described)
Something: We'll have to invoke the _complete command for this
Nothing: Skip completion, this option has no value (or value is not autocompletable).

We can even extend this with e.g. Files, to enable the great file completion system of shell completion.

@GromNaN GromNaN force-pushed the console-completion-definition branch 2 times, most recently from 0f93398 to f8e03db Compare January 8, 2022 12:15
@GromNaN
Copy link
Member Author

GromNaN commented Jan 10, 2022

After you comment @wouterj, I think we can use a union type that covers each cases.

  • true: calling complete.
  • false: disable completion
  • \Closure: call it
  • iterable: get fixed values from the iterable
  • null: not defined, will call complete by default. Setting null should be deprecated and become false by default in 7.0.

We must stay open to file completion once we'd have managed to get it work #43607.

src/Symfony/Component/Console/Command/Command.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Command/Command.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Command/Command.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Input/InputArgument.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Input/InputArgument.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Command/Command.php Outdated Show resolved Hide resolved
@GromNaN GromNaN force-pushed the console-completion-definition branch from f8e03db to c4d65b3 Compare February 13, 2022 22:07
@GromNaN GromNaN force-pushed the console-completion-definition branch from c4d65b3 to ab40533 Compare February 14, 2022 08:58
@GromNaN GromNaN force-pushed the console-completion-definition branch 2 times, most recently from e668ed4 to 0e31423 Compare February 14, 2022 14:20
src/Symfony/Component/Console/Command/Command.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Command/Command.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Command/LazyCommand.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Input/InputArgument.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Input/InputArgument.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Input/InputOption.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Input/InputOption.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Input/InputOption.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Input/InputArgument.php Outdated Show resolved Hide resolved
@GromNaN GromNaN force-pushed the console-completion-definition branch 4 times, most recently from d1a2af5 to 80654e5 Compare February 15, 2022 14:58
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Much better :)

src/Symfony/Component/Console/Command/LazyCommand.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Input/InputOption.php Outdated Show resolved Hide resolved
@GromNaN GromNaN force-pushed the console-completion-definition branch from 80654e5 to 5d70c9b Compare February 15, 2022 18:30
@GromNaN
Copy link
Member Author

GromNaN commented Feb 15, 2022

Thank you for all your time for this review. We have something very clear now.

@GromNaN GromNaN force-pushed the console-completion-definition branch 3 times, most recently from 091027a to 4f9c779 Compare March 16, 2022 18:43
@GromNaN
Copy link
Member Author

GromNaN commented Mar 16, 2022

@chalasr @wouterj ready for a new review. Description updated.

Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

Great. Just one minor question

src/Symfony/Component/Console/Command/LazyCommand.php Outdated Show resolved Hide resolved
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Some last minute nitpicking;

src/Symfony/Component/Console/Input/InputOption.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Input/InputArgument.php Outdated Show resolved Hide resolved
src/Symfony/Component/Console/Command/Command.php Outdated Show resolved Hide resolved
@GromNaN GromNaN force-pushed the console-completion-definition branch 3 times, most recently from ae8e882 to a2a2e67 Compare March 17, 2022 15:56
@GromNaN GromNaN force-pushed the console-completion-definition branch from a2a2e67 to 95ad05f Compare March 17, 2022 16:09
@nicolas-grekas
Copy link
Member

What about this todo?

Add info to descriptor

@GromNaN
Copy link
Member Author

GromNaN commented Mar 17, 2022

What about this todo?

Add info to descriptor

Could be part of an other PR.

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.

None yet

6 participants