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

bash-completion: update and improve supported completion cases #4559

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

kubiko
Copy link
Contributor

@kubiko kubiko commented Feb 4, 2024

multiple improvements to the completion logic

  • add multiple new commands,
  • replace snap with pack
  • add supported architectures for --target-arch and --build-for
    • do not parse snapcraft.yaml as the format changes between core20 and core22 bases, and we have only a list of predefined options regardless, use those.
  • build | clean | pull | stage | prime: parse snapcraft.yaml for suggested parts
    • use shell/sed based custom parser. we do not want to bring yq as an extra dependency, and calling back to the python parser demonstrated to be very slow

multiple improvements to the completion logic
- add multiple new commands,
- replace `snap` with `pack`
- add supported architectures for `--target-arch` and `--build-for`
  - do not parse snapcraft.yaml as format changes between core20 and core22
    bases, and we have only a list of predefined options regardless, use those.
- build | clean | pull | stage | prime: parse snapcraft.yaml for suggested parts
  - use shell/sed based custom parser. we do not want to bring yq as an extra
    dependency, and calling back to python parser demonstrated to be very slow

Signed-off-by: Ondrej Kubik <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Feb 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (89961cf) 89.18% compared to head (5b5a29c) 88.33%.
Report is 3 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4559      +/-   ##
==========================================
- Coverage   89.18%   88.33%   -0.85%     
==========================================
  Files         321      327       +6     
  Lines       21777    21961     +184     
==========================================
- Hits        19421    19399      -22     
- Misses       2356     2562     +206     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kubiko
Copy link
Contributor Author

kubiko commented Feb 4, 2024

@cmatsuoka @sergiusens I finally for a moment to revisit this.
Given that yq is out as outside dependency, and callback to python as tested by Claudio was very slow, I went back to shell option. Any shell-based yaml parsers I could find are overkill and usually are also a bit slow. Processing snapcraft.yaml line by line was also dog slow. So while admittedly not as readable, sed seems to be the way.
I am sure I did not catch all the corner cases, but so far this has worked on every snapcraft.yaml I could throw at it,

@lengau lengau self-requested a review February 6, 2024 16:58
@cmatsuoka cmatsuoka requested a review from mr-cal February 6, 2024 17:00
debian/snapcraft-completion Outdated Show resolved Hide resolved
debian/snapcraft-completion Outdated Show resolved Hide resolved
@@ -6,15 +20,15 @@ _snapcraft()
COMPREPLY=()
cur="${COMP_WORDS[COMP_CWORD]}"
prev="${COMP_WORDS[COMP_CWORD-1]}"
opts="help init list-plugins plugins login logout export-login list-keys keys create-key register-key register registered list-registered push release clean cleanbuild pull build sign-build stage prime snap update define search gated validate history status close enable-ci expand-extensions extension extensions list-extensions"
opts="help init list-plugins plugins login logout export-login list-keys keys create-key register-key register registered list-registered push release clean cleanbuild pull build sign-build stage prime pack update define search gated validate history status close enable-ci expand-extensions extension extensions list-extensions --target-arch --build-for --debug --shell --shell-after ---provider -h --help --verbose"
Copy link
Contributor

Choose a reason for hiding this comment

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

Most of these new parameters should only be available if a lifecycle command has already been declared. For example, snapcraft plugins --build-for isn't valid

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lengau that is fair point, let me see if this can be fixed in easy way

Copy link
Contributor

@lengau lengau Feb 15, 2024

Choose a reason for hiding this comment

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

Probably the easiest way would be to check whether any of the lifecycle commands are in $COMP_WORDS:

for i in "${COMP_WORDS}"; do
 case "$i" in  
  pull);&
  build);&
  stage);&
  prime);&
  pack);& 
  clean) 
    opts="${opts} --build-for --target-arch --debug --shell --shell-after --provider"
    break
 esac
done

Copy link
Contributor

Choose a reason for hiding this comment

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

@kubiko any update?

Copy link

codecov bot commented Mar 19, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.36%. Comparing base (057e21b) to head (fb9de45).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4559   +/-   ##
=======================================
  Coverage   88.36%   88.36%           
=======================================
  Files         327      327           
  Lines       21997    21997           
=======================================
  Hits        19438    19438           
  Misses       2559     2559           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

Successfully merging this pull request may close these issues.

3 participants