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

feat(dispatcher): constrained global arguments #220

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

lengau
Copy link
Contributor

@lengau lengau commented Jan 12, 2024

Fixes #219

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run tox?

@lengau lengau force-pushed the constrained-global-args branch 2 times, most recently from 6011c94 to 84e951b Compare January 12, 2024 18:10
Copy link

codecov bot commented Jan 12, 2024

Codecov Report

Attention: Patch coverage is 97.72727% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 94.85%. Comparing base (99394c9) to head (8316c77).
Report is 7 commits behind head on main.

❗ Current head 8316c77 differs from pull request most recent head ca5e5c3. Consider uploading reports for the commit ca5e5c3 to get more accurate results

Files Patch % Lines
craft_cli/dispatcher.py 97.29% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #220      +/-   ##
==========================================
+ Coverage   93.24%   94.85%   +1.61%     
==========================================
  Files           7        8       +1     
  Lines        1080     1107      +27     
  Branches      200      211      +11     
==========================================
+ Hits         1007     1050      +43     
+ Misses         66       53      -13     
+ Partials        7        4       -3     

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

Comment on lines +120 to +121
validator=lambda mode: EmitterMode[mode.upper()],
case_sensitive=False,
Copy link
Contributor

Choose a reason for hiding this comment

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

choices also exists in argparse, will validator and case_sensitive options increase the gap between global argument handling and command-specific argument handling instead of reducing it? Ideally we should have an uniform way to handle all arguments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

validator is equivalent to argparse's type - I can rename if you'd like.

case_sensitive is different, but is there for backwards compatibility in the way we handle --verbosity, which was previously case-insensitive due to special handling. Looking over it again, I wonder if I could just run the validator before limiting choices and make a special verbosity_validator function that returns the correct emitter mode.

@lengau lengau requested a review from cmatsuoka April 12, 2024 22:13
@lengau lengau marked this pull request as draft September 13, 2024 13:07
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.

Provide limited choices for global arguments
2 participants