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

fix(cli): allow exclusive arguments as optional #2770

Merged
merged 3 commits into from
Jan 29, 2024

Conversation

Sjord
Copy link
Contributor

@Sjord Sjord commented Jan 21, 2024

The CLI takes its arguments from the RequiredOptional, which has three fields: required, optional, and exclusive. In practice, the exclusive options are not defined as either required or optional, and would not be allowed in the CLI. This changes that, so that exclusive options are also added to the argument parser.

Closes #2769

Changes

Documentation and testing

Please consider whether this PR needs documentation and tests. This is not required, but highly appreciated:

Copy link

codecov bot commented Jan 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (48726fd) 96.48% compared to head (e435fc9) 96.49%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2770   +/-   ##
=======================================
  Coverage   96.48%   96.49%           
=======================================
  Files          90       90           
  Lines        5866     5874    +8     
=======================================
+ Hits         5660     5668    +8     
  Misses        206      206           
Flag Coverage Δ
api_func_v4 82.20% <0.00%> (-0.12%) ⬇️
cli_func_v4 83.55% <62.50%> (-0.03%) ⬇️
unit 88.27% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Coverage Δ
gitlab/v4/cli.py 91.69% <100.00%> (+0.22%) ⬆️

@JohnVillalovos
Copy link
Member

Would it make more sense to create a mutually exclusive group and add the exclusive args to that?

https://docs.python.org/3/library/argparse.html#mutual-exclusion

@Sjord
Copy link
Contributor Author

Sjord commented Jan 28, 2024

I've changed the code to create a mutually exclusive group.

I think the data model of RequiredOptional is not ideal. Whether options are exclusive should be perpendicular to whether they are required or optional. Also, it would be possible to have multiple exclusive groups. Currently this is not reflected in the RequiredOptional.exclusive property. It only allows one group, and it is not clear whether this group is required or not.

Ideally, you would have something lilke this:

RequiredOptional(
    required=("foo", exclusive("user_id", "user_email"), exclusive("project_slug", "project_id")),
    optional=("bar", exclusive("cn", "filter"))
)

But adding these exclusive options to the CLI argument parser at least makes it functioning again, if not perfect.

gitlab/v4/cli.py Outdated Show resolved Hide resolved
The CLI takes its arguments from the RequiredOptional, which has three fields: required, optional, and exclusive. In practice, the exclusive options are not defined as either required or optional, and would not be allowed in the CLI. This changes that, so that exclusive options are also added to the argument parser.

Closes python-gitlab#2769
Copy link
Member

@JohnVillalovos JohnVillalovos left a comment

Choose a reason for hiding this comment

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

Thank you very much for this.

@JohnVillalovos JohnVillalovos merged commit 7ec3189 into python-gitlab:main Jan 29, 2024
16 checks passed
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.

Exclusive arguments not allowed in CLI
2 participants