-
Notifications
You must be signed in to change notification settings - Fork 91
fix: Use Click v8.x native shell completion #2639
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
base: main
Are you sure you want to change the base?
fix: Use Click v8.x native shell completion #2639
Conversation
matthewfeickert
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tab completions are currently very slow, so I'm not sure what's happening there.
Some of this is LLM generated to get a working scaffolding, so this shouldn't be merged until things are better understood and the docs are updated significantly.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2639 +/- ##
==========================================
- Coverage 98.23% 98.23% -0.01%
==========================================
Files 65 65
Lines 4190 4188 -2
Branches 453 454 +1
==========================================
- Hits 4116 4114 -2
Misses 45 45
Partials 29 29
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
From the docs:
If you generate it ahead of time, then it won't be as slow, but it's still going to be slow. |
This is actually how I was testing it, I just wrote the docs to be more trivial. Though I guess we should give people the best shot at having things not be horribly slow, so I should revise this. |
|
From @henryiii:
|
63a065d to
0f7d17c
Compare
|
@kratsg This is ready for review. My suggestion is that even though we know this is slow for the time being (though would still be good to get your check on this also) that we move forward with it so that we can drop an outdated and deprecated dependency and extra`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR migrates from the external click_completion library to Click v8.x's native shell completion functionality, simplifying the dependency structure and modernizing the shell completion implementation.
Key changes:
- Replaced click_completion dependency with Click v8.x native shell completion API
- Removed the
shellcompleteoptional dependency extra from pyproject.toml - Updated CLI tests to verify the new completion implementation for bash, zsh, and fish shells
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/pyhf/cli/complete.py | Migrated from click_completion to Click v8.x native completion API with inline documentation for bash, zsh, and fish shells |
| tests/test_cli.py | Replaced legacy tests with new tests for bash, zsh, and fish completion instructions, plus a test for no-shell invocation |
| pyproject.toml | Removed shellcomplete extra and updated the "all" extra to exclude it |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/pyhf/cli/complete.py
Outdated
| Bash: | ||
| mkdir -p ~/.completions | ||
| _PYHF_COMPLETE=bash_source pyhf > ~/.completions/pyhf-complete.sh | ||
| echo ". ~/.completions/pyhf-complete.sh" >> ~/.bashrc |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The docstring instructions don't match the actual command output. The docstring shows:
echo ". ~/.completions/pyhf-complete.sh" >> ~/.bashrc
But the actual command output includes -e flag and \\n:
echo -e "\\n. ~/.completions/pyhf-complete.sh" >> ~/.bashrc
These should be consistent. Either update the docstring to match the output, or simplify the output to match the docstring.
0f7d17c to
65f1163
Compare
* Use click v8.x's native shell completions to produce pyhf shell completions for the CLI API. - c.f. https://click.palletsprojects.com/en/stable/shell-completion/ * Remove shellcomplete extra. * Update tests for CLI API. * Extend documentation for producing shell completions for Bash, Zsh, and Fish shells.
65f1163 to
ddf9fde
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Supported shells: bash, zsh, fish | ||
| To enable completion, run the appropriate command for your shell: | ||
| \b | ||
| Bash: | ||
| mkdir -p ~/.completions | ||
| _PYHF_COMPLETE=bash_source pyhf > ~/.completions/pyhf-complete.sh | ||
| echo -e "\n. ~/.completions/pyhf-complete.sh" >> ~/.bashrc | ||
| \b | ||
| Zsh: | ||
| mkdir -p ~/.completions | ||
| _PYHF_COMPLETE=zsh_source pyhf > ~/.completions/pyhf-complete.zsh | ||
| echo -e "\n. ~/.completions/pyhf-complete.zsh" >> ~/.zshrc | ||
| \b | ||
| Fish: | ||
| _PYHF_COMPLETE=fish_source pyhf >> ~/.config/fish/completions/pyhf.fish |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shell completion instructions are duplicated in both the docstring (lines 20-33) and the instructions dictionary (lines 42-55). This creates a maintenance burden where updates must be made in two places. Consider removing the detailed instructions from the docstring and keeping only the high-level description, or generate the help text dynamically from the instructions dictionary.
| Supported shells: bash, zsh, fish | |
| To enable completion, run the appropriate command for your shell: | |
| \b | |
| Bash: | |
| mkdir -p ~/.completions | |
| _PYHF_COMPLETE=bash_source pyhf > ~/.completions/pyhf-complete.sh | |
| echo -e "\n. ~/.completions/pyhf-complete.sh" >> ~/.bashrc | |
| \b | |
| Zsh: | |
| mkdir -p ~/.completions | |
| _PYHF_COMPLETE=zsh_source pyhf > ~/.completions/pyhf-complete.zsh | |
| echo -e "\n. ~/.completions/pyhf-complete.zsh" >> ~/.zshrc | |
| \b | |
| Fish: | |
| _PYHF_COMPLETE=fish_source pyhf >> ~/.config/fish/completions/pyhf.fish | |
| Supported shells: bash, zsh, fish. | |
| When run, this command will print instructions for enabling shell completion | |
| for the specified shell. |
| bold=True, | ||
| ) | ||
| ) | ||
| click.echo("and then source your shell configuration or restart your shell.") |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing capitalization at the start of the sentence. The sentence should begin with a capital letter for consistency with standard English grammar.
| click.echo("and then source your shell configuration or restart your shell.") | |
| click.echo("And then source your shell configuration or restart your shell.") |
Description
Resolves #2622
Checklist Before Requesting Reviewer
Before Merging
For the PR Assignees: