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

Add completion to commands options and arguments #10320

Merged
merged 15 commits into from Jun 1, 2022

Conversation

GromNaN
Copy link
Contributor

@GromNaN GromNaN commented Nov 30, 2021

Fix #10292

How to use ?

Install bash and bash_completion on your system.

# Ubuntu
sudo apt install bash-completion
# Mac
brew install bash bash-completion@2

Init completion script

bin/composer completion bash > completion.bash
source completion.bash
bin/composer <tab,tab>

Code requires symfony/console >= 5.4 to work and PHP >= 7.1 to not fail (void return type). The dependency updates are only for testing.

Note: for testing I use the debug command described here symfony/symfony#43594 (comment)

composer.json Outdated Show resolved Hide resolved
composer.json Outdated Show resolved Hide resolved
@Seldaek Seldaek added this to the 2.3 milestone Nov 30, 2021
Copy link
Member

@Seldaek Seldaek left a comment

Choose a reason for hiding this comment

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

Thanks for getting started on this, looking forward to having it available!

src/Composer/Command/DependsCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/ExecCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/OutdatedCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/OutdatedCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/RemoveCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/RequireCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/ShowCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/ShowCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/UpdateCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/UpdateCommand.php Outdated Show resolved Hide resolved
Copy link

@dkarlovi dkarlovi left a comment

Choose a reason for hiding this comment

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

Amazing work! 🚀

src/Composer/Command/RequireCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/BaseCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/BaseCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/BaseCommand.php Outdated Show resolved Hide resolved
src/Composer/Command/BaseCommand.php Outdated Show resolved Hide resolved
Copy link

@dkarlovi dkarlovi left a comment

Choose a reason for hiding this comment

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

Hands on experience.

src/Composer/Command/RequireCommand.php Outdated Show resolved Hide resolved

$packages = $repos->search('^'.$input->getCompletionValue(), RepositoryInterface::SEARCH_NAME);

foreach (array_slice($packages, 0, 150) as $package) {
Copy link

Choose a reason for hiding this comment

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

The fact that we're limiting to 150 here has non-obvious consequences when used, see #10325. This part needs some special attention to polish the UX here.

Copy link
Member

Choose a reason for hiding this comment

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

I did improve this in 55dc808 - for vendor names though when going through packagist.org results there are still so many that it's kind of a mess and you'll in most cases have to type enough of what you want to get it, but then it completes packages names within the vendor which is already quite useful IMO.

@GromNaN GromNaN force-pushed the command-completion branch 4 times, most recently from deb29af to dec43d6 Compare December 7, 2021 15:50
@private-packagist
Copy link

The composer.lock diff comment has been updated to reflect new changes in this PR.

@Seldaek Seldaek mentioned this pull request Dec 7, 2021
Seldaek added a commit to Seldaek/composer that referenced this pull request Dec 7, 2021
Seldaek added a commit that referenced this pull request Dec 8, 2021
* Search performance improvements, add SEARCH_VENDOR type, fixes #10326, fixes #10324, fixes #10325

* Add extra optimization path for autocompletion of ^foo/* whereas the vendor is fully known, refs #10320
@Seldaek
Copy link
Member

Seldaek commented Jan 3, 2022

Please rebase on latest main branch :)

@GromNaN
Copy link
Contributor Author

GromNaN commented Jan 3, 2022

I rebased, fixed some phpstan warnings.

$suggestions->suggestValues(array_filter(array_map(function (Command $command) {
return $command->isHidden() ? null : $command->getName();
}, $application->all())));
}
Copy link
Contributor Author

@GromNaN GromNaN Jan 3, 2022

Choose a reason for hiding this comment

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

Completion of subcommand is hard. I have this for beginning; but the global state of the application must be modified.

        if ($application->has($commandName = $input->getArgument('command-name'))) {
            $input = CompletionInput::fromString(preg_replace(['{\bg(?:l(?:o(?:b(?:a(?:l)?)?)?)?)?\b}', '{|$}'], ['', ''], $input->__toString(), 1), 3);
            $command = $this->getApplication()->find($commandName);
            $input->bind($command->getDefinition());
            $command->complete($input, $suggestions);
        }

Copy link
Member

Choose a reason for hiding this comment

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

Not sure what you mean by "the global state of the application must be modified"?

Copy link
Contributor Author

@GromNaN GromNaN Jan 6, 2022

Choose a reason for hiding this comment

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

All the things that are done in the run method.

// The COMPOSER env var should not apply to the global execution scope
if (Platform::getEnv('COMPOSER')) {
Platform::clearEnv('COMPOSER');
}
// change to global dir
$config = Factory::createConfig();
$home = $config->get('home');
if (!is_dir($home)) {
$fs = new Filesystem();
$fs->ensureDirectoryExists($home);
if (!is_dir($home)) {
throw new \RuntimeException('Could not create home directory');
}
}
try {
chdir($home);
} catch (\Exception $e) {
throw new \RuntimeException('Could not switch to home directory "'.$home.'"', 0, $e);
}
$this->getIO()->writeError('<info>Changed current directory to '.$home.'</info>');
// create new input without "global" command prefix
$input = new StringInput(Preg::replace('{\bg(?:l(?:o(?:b(?:a(?:l)?)?)?)?)?\b}', '', $input->__toString(), 1));
$this->getApplication()->resetComposer();

That said ... that's not so complex.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh I see. Yes that should be easy to extract in a method to avoid duplication?

Copy link
Member

Choose a reason for hiding this comment

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

See dd2e2d7 - I tried doing it but it doesn't seem to work, I think I missed something, perhaps you can have a look?

@Seldaek Seldaek removed this from the 2.3 milestone Feb 18, 2022
src/Composer/Factory.php Outdated Show resolved Hide resolved
@Seldaek
Copy link
Member

Seldaek commented May 13, 2022

OK PR is good to merge for me now except for the glitches in the global command. @GromNaN do you think you have time to look at that soon? Otherwise I'd probably merge as is and it can be done in a follow-up PR.

chalasr added a commit to symfony/symfony that referenced this pull request May 13, 2022
This PR was merged into the 5.4 branch.

Discussion
----------

Fix aliases handling in command name completion

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | Fix #... <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

While working on composer/composer#10320 I noticed that command aliases like `composer why` did not autocomplete and then even if typed manually the args/options did not autocomplete if using the alias. This PR fixes it.

Commits
-------

8ffc015 Fix aliases handling in command name completion
@GromNaN
Copy link
Contributor Author

GromNaN commented May 16, 2022

OK PR is good to merge for me now except for the glitches in the global command. @GromNaN do you think you have time to look at that soon? Otherwise I'd probably merge as is and it can be done in a follow-up PR.

Completion on global command needs some hooks in the Composer\Console\Application. I don't have anything conclusive ATM. Better work on it in a separate PR.

@GromNaN
Copy link
Contributor Author

GromNaN commented May 16, 2022

Next step will be the setup the completion script with composer.phar.
This line may be removed.

->notPath('symfony/console/Resources/completion.bash')

@Seldaek Seldaek merged commit ef06702 into composer:main Jun 1, 2022
@Seldaek
Copy link
Member

Seldaek commented Jun 1, 2022

Thanks @GromNaN! Let's look at the global command in a further PR then whenever you have a chance :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[2.3] Add support for completion when on Symfony 5.4
4 participants