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

ArgGroups values don't work well with get_many #3748

Closed
epage opened this issue May 24, 2022 · 3 comments · Fixed by #4072
Closed

ArgGroups values don't work well with get_many #3748

epage opened this issue May 24, 2022 · 3 comments · Fixed by #4072
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Milestone
4.0

Comments

@epage
Copy link
Member

epage commented May 24, 2022 •
edited
Loading

When adding args, we also add them to their groups in ArgMatches but with #3732, we fail if the group has disparate types.

As an aside, people are paying for this issue when most aren't using it (especially derive).

We should improve this situation
@epage epage added C-bug Category: Updating dependencies S-waiting-on-decision Status: Waiting on a go/no-go before implementing A-parsing Area: Parser's logic and needs it changed somehow. labels May 24, 2022
@epage
Copy link
Member Author

epage commented May 24, 2022 •
edited
Loading

  • We should allow setting an Action on arg groups so people opt-in to them appending, inspired by actions for args in Confusing how to override version or helps behavior #3405
  • We should have a debug assert on the data type being the same for group members if the action has it being assigned

@epage epage added the M-breaking-change Meta: Implementing or merging this will introduce a breaking change. label May 24, 2022
@epage epage added this to the 4.0 milestone May 24, 2022
@epage
Copy link
Member Author

epage commented May 25, 2022

ArgGroup actions might be a way of resolving #2317 as well

@epage epage mentioned this issue May 25, 2022
Closed
2 tasks
@epage
Copy link
Member Author

epage commented Jul 22, 2022

When fixing this, we could revert part of #3972

epage added a commit to epage/clap that referenced this issue Aug 12, 2022
@epage
fix(parser)!: Store args in a group, rather than values
0df00ad 
Now that `Id` is public, we can have `ArgMatches` report them.  If we
have to choose one behavior, this is more universal.  The user can still
look up the values, this works with groups whose args have different
types, and this allows people to make decisions off of it when otherwise
there isn't enogh information.

Fixes clap-rs#2317
Fixes clap-rs#3748
epage added a commit to epage/clap that referenced this issue Aug 12, 2022
@epage
fix(parser)!: Store args in a group, rather than values
41be1be 
Now that `Id` is public, we can have `ArgMatches` report them.  If we
have to choose one behavior, this is more universal.  The user can still
look up the values, this works with groups whose args have different
types, and this allows people to make decisions off of it when otherwise
there isn't enogh information.

Fixes clap-rs#2317
Fixes clap-rs#3748
@epage epage mentioned this issue Aug 12, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-waiting-on-decision Status: Waiting on a go/no-go before implementing
Projects
None yet
Milestone
4.0
Development

Successfully merging a pull request may close this issue.

fix(parser)!: Store args in a group, rather than values epage/clap
1 participant
@epage