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.

Sign up for GitHub

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

Jump to bottom

feat: Enable shell completion #919

Merged
merged 17 commits into from
Jul 11, 2020
Merged

feat: Enable shell completion #919

merged 17 commits into from
Jul 11, 2020

Conversation

lukasheinrich
Copy link
Contributor

@lukasheinrich lukasheinrich commented Jul 3, 2020 •
edited by matthewfeickert
Loading

Description

do we want to add shell completion?

works like this

eval "$(pyhf shell-completion)"
pyhf c<tab><tab>
cls      combine

Checklist Before Requesting Reviewer

  • Tests are passing
  • "WIP" removed from the title of the pull request
  • Selected an Assignee for the PR to be responsible for the log summary

Before Merging

For the PR Assignees:

  • Summarize commit messages into a comprehensive review of the PR
* Add shell completion with click_completion
* Add tests for the CLI

@codecov
Copy link

codecov bot commented Jul 3, 2020 •
edited
Loading

Codecov Report

Merging #919 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #919      +/-   ##
==========================================
+ Coverage   96.30%   96.31%   +0.01%     
==========================================
  Files          56       57       +1     
  Lines        3191     3204      +13     
  Branches      438      438              
==========================================
+ Hits         3073     3086      +13     
  Misses         75       75              
  Partials       43       43
Flag Coverage Δ
#unittests 96.31% <100.00%> (+0.01%) ⬆️
Impacted Files Coverage Δ
src/pyhf/cli/__init__.py 100.00% <100.00%> (ø)
src/pyhf/cli/cli.py 100.00% <100.00%> (ø)
src/pyhf/cli/complete.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3d3ead...45cd6dd. Read the comment docs.

@matthewfeickert matthewfeickert added CLI Affects the CLI API feat/enhancement New feature or request labels Jul 3, 2020
@matthewfeickert matthewfeickert marked this pull request as draft July 3, 2020 23:39
@matthewfeickert matthewfeickert changed the title [WIP] shell completion feat: Enable shell completion Jul 3, 2020
@matthewfeickert
Copy link
Member

do we want to add shell completion?

Yes please! 😄

@kratsg
Copy link
Contributor

kratsg commented Jul 4, 2020

it would be great to... ... test it somehow(???). Can it be done without an eval (e.g. via pip install.)? https://pip.pypa.io/en/stable/user_guide/#command-completion

@matthewfeickert
Copy link
Member

Also, is the current PR supposed to work yet? On my machine I get

$ git checkout shellcompletion 
$ eval "$(pyhf shell-completion)"
Usage: pyhf [OPTIONS] COMMAND [ARGS]...
Try 'pyhf -h' for help.

Error: No such command 'shell-completion'.

@lukasheinrich
Copy link
Contributor Author

ok I added a test (that checks the stdout of the pyhf shellcomplete bash command) this reminds me that we should probably generally test the CLI in some ways (without testing the base APIs all over again)

@lukasheinrich
Copy link
Contributor Author

@matthewfeickert I added the setup.py dependencies

lukasheinrich
lukasheinrich commented Jul 8, 2020
View reviewed changes
src/pyhf/cli/cli.py Outdated Show resolved Hide resolved
@lukasheinrich lukasheinrich marked this pull request as ready for review July 8, 2020 21:33
kratsg
kratsg requested changes Jul 9, 2020
View reviewed changes
setup.py Show resolved Hide resolved
setup.py Show resolved Hide resolved
src/pyhf/cli/cli.py Outdated Show resolved Hide resolved
@kratsg kratsg requested a review from matthewfeickert July 11, 2020 20:12
@matthewfeickert matthewfeickert added the tests pytest label Jul 11, 2020
matthewfeickert
matthewfeickert approved these changes Jul 11, 2020
View reviewed changes
Copy link
Member

@matthewfeickert matthewfeickert left a comment

Choose a reason for hiding this comment

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

LGTM. Nice work both of you! :)

@kratsg kratsg merged commit 1da2d6e into master Jul 11, 2020
@kratsg kratsg deleted the shellcompletion branch July 11, 2020 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewfeickert matthewfeickert matthewfeickert approved these changes

@kratsg kratsg kratsg approved these changes

Assignees

@lukasheinrich lukasheinrich

Labels
CLI Affects the CLI API feat/enhancement New feature or request tests pytest
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lukasheinrich @matthewfeickert @kratsg