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

Stablize ArgMatches::grouped_value_of Tracking Issue #2924

Closed
4 tasks done
epage opened this issue Oct 23, 2021 · 10 comments · Fixed by #4635
Closed
4 tasks done

Stablize ArgMatches::grouped_value_of Tracking Issue #2924

epage opened this issue Oct 23, 2021 · 10 comments · Fixed by #4635
Assignees
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-tracking-issue Category: A tracking issue for an unstable feature S-blocked Status: Blocked on something else such as an RFC or other implementation work.

Comments

@epage
Copy link
Member

epage commented Oct 23, 2021

Original request: #1026

Original PR: #2297

Feature flag: unstable-grouped

Known issues

@epage epage self-assigned this Oct 23, 2021
epage added a commit to epage/clap that referenced this issue Oct 23, 2021
There is enough open work on this, we should probably not have it public
yet, so putting it behind a gate.  See clap-rs#2924
epage added a commit to epage/clap that referenced this issue Oct 23, 2021
There is enough open work on this, we should probably not have it public
yet, so putting it behind a gate.  See clap-rs#2924
epage added a commit to epage/clap that referenced this issue Oct 25, 2021
There is enough open work on this, we should probably not have it public
yet, so putting it behind a gate.  See clap-rs#2924
epage added a commit to epage/clap that referenced this issue Oct 26, 2021
There is enough open work on this, we should probably not have it public
yet, so putting it behind a gate.  See clap-rs#2924
@pksunkara pksunkara added C-tracking-issue Category: A tracking issue for an unstable feature C: matches labels Oct 27, 2021
@epage
Copy link
Member Author

epage commented Nov 4, 2021

For _t and _os variants, should we wait on #2683 which will remove the need for them? Or at least not block this feature on them?

@pksunkara
Copy link
Member

pksunkara commented Nov 5, 2021

I think that's a good idea, since I am getting the impression that we will probably be working on #2683 as the next big thing after v3 release. Unless you have different plans?

@epage
Copy link
Member Author

epage commented Nov 5, 2021

Unless you have different plans?

That and a couple other changes that will lay the ground work for help, parser, and other validation changes. I don't think we need to cram them all in but should keep an eye for changes that make it easier to get them in later. #2911 is one example because it means we can attach the reflection traits to the AppParser without making them &mut to allow building since the type system will enforce that building has occurred.

epage added a commit to epage/clap that referenced this issue Nov 5, 2021
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 1, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple occurrences, optional
  - optional: `Vec` implies 0 or more, so should not imply required
- `Option<Vec<_>>`: multiple occurrences, optional
  - optional: Use over `Vec` to detect when no option being present when
    using multiple values

Motivations:

My priorities were:
1. Are we getting in the users way?
2. Does the API make sense?
3. Does the API encourage best practices?

I was originally concerned about the lack of composability with
`Option<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<Vec<_>>`).
It prescribes special meaning to each type depending on where it shows
up, rather than providing a single meaning for a type generally.  You
then can't do things like have `Option<_>` mean "required argument with
optional value" without hand constructing it.  However, in practice the
outer type correlates with the argument occurrence and the inner type with the
value.   It is rare to want the value behavior without also the
occurrence behavior.  So I figure it is probably fine as long as people
can set the flags to manually get the behavior they want.

`Vec<_>` implies multiple occurrences, rather than multiple values.
Anecdotally, whenver I've used the old `Arg::multiple`, I thought I was
getting `Arg::multiple_occurrences` only.  `Arg::multiple_values`,
without any bounds or delimeter requirement, can lead to a confusing
user experience but isn't a good default for these.  On top of that, if
someone does have an unbounded or a delimeter multiple values, they are
probably also using multiple occurrences.

`Vec<_>` is optional because a `Vec` implies 0 or more, so we stick to
the meaning of the rust type.

`Option<Vec<_>>` ends up matching `Vec<_>` which an raise the question
of why have it.  Some users might prefer the type.  Otherwise, this is
so users can no when the argument is present or not when using
`min_values(0)`.  Rather than defining an entire policy around this and
having users customize it, or setting `min_values(0)` without the rest
of a default policy, this gives people a blank slate to work from.

Another option would have been to not infer a setting if someone sets a
handful of settings manually, which would have avoided the confusion in
Issue clap-rs#2599 but I see that being confusing (for someone who knows the
default, they will be expecting it to be additive; which flags?) and
brittle (as flags are added or changed, how do we ensure we keep this
up?)

Tests were added to ensure we support people customizing the behavior to
match their needs.

This is not solving:
- `Vec<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
epage added a commit to epage/clap that referenced this issue Nov 5, 2021
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 0, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple occurrences, optional
  - optional: `Vec` implies 0 or more, so should not imply required
- `Option<Vec<_>>`: multiple occurrences, optional
  - optional: Use over `Vec` to detect when no option being present when
    using multiple values

Motivations:

My priorities were:
1. Are we getting in the users way?
2. Does the API make sense?
3. Does the API encourage best practices?

I was originally concerned about the lack of composability with
`Option<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<Vec<_>>`).
It prescribes special meaning to each type depending on where it shows
up, rather than providing a single meaning for a type generally.  You
then can't do things like have `Option<_>` mean "required argument with
optional value" without hand constructing it.  However, in practice the
outer type correlates with the argument occurrence and the inner type with the
value.   It is rare to want the value behavior without also the
occurrence behavior.  So I figure it is probably fine as long as people
can set the flags to manually get the behavior they want.

`Vec<_>` implies multiple occurrences, rather than multiple values.
Anecdotally, whenver I've used the old `Arg::multiple`, I thought I was
getting `Arg::multiple_occurrences` only.  `Arg::multiple_values`,
without any bounds or delimeter requirement, can lead to a confusing
user experience but isn't a good default for these.  On top of that, if
someone does have an unbounded or a delimeter multiple values, they are
probably also using multiple occurrences.

`Vec<_>` is optional because a `Vec` implies 0 or more, so we stick to
the meaning of the rust type.

`Option<Vec<_>>` ends up matching `Vec<_>` which an raise the question
of why have it.  Some users might prefer the type.  Otherwise, this is
so users can no when the argument is present or not when using
`min_values(0)`.  Rather than defining an entire policy around this and
having users customize it, or setting `min_values(0)` without the rest
of a default policy, this gives people a blank slate to work from.

Another option would have been to not infer a setting if someone sets a
handful of settings manually, which would have avoided the confusion in
Issue clap-rs#2599 but I see that being confusing (for someone who knows the
default, they will be expecting it to be additive; which flags?) and
brittle (as flags are added or changed, how do we ensure we keep this
up?)

Tests were added to ensure we support people customizing the behavior to
match their needs.

This is not solving:
- `Vec<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
epage added a commit to epage/clap that referenced this issue Nov 5, 2021
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 0, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple occurrences, optional
  - optional: `Vec` implies 0 or more, so should not imply required
- `Option<Vec<_>>`: multiple occurrences, optional
  - optional: Use over `Vec` to detect when no option being present when
    using multiple values

Motivations:

My priorities were:
1. Are we getting in the users way?
2. Does the API make sense?
3. Does the API encourage best practices?

I was originally concerned about the lack of composability with
`Option<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<Vec<_>>`).
It prescribes special meaning to each type depending on where it shows
up, rather than providing a single meaning for a type generally.  You
then can't do things like have `Option<_>` mean "required argument with
optional value" without hand constructing it.  However, in practice the
outer type correlates with the argument occurrence and the inner type with the
value.   It is rare to want the value behavior without also the
occurrence behavior.  So I figure it is probably fine as long as people
can set the flags to manually get the behavior they want.

`Vec<_>` implies multiple occurrences, rather than multiple values.
Anecdotally, whenever I've used the old `Arg::multiple`, I thought I was
getting `Arg::multiple_occurrences` only.  `Arg::multiple_values`,
without any bounds or delimiter requirement, can lead to a confusing
user experience and isn't a good default for these.  On top of that, if
someone does have an unbounded or a delimiter multiple values, they are
probably also using multiple occurrences.

`Vec<_>` is optional because a `Vec` implies 0 or more, so we stick to
the meaning of the rust type.

`Option<Vec<_>>` ends up matching `Vec<_>` which an raise the question
of why have it.  Some users might prefer the type.  Otherwise, this is
so users can no when the argument is present or not when using
`min_values(0)`.  Rather than defining an entire policy around this and
having users customize it, or setting `min_values(0)` without the rest
of a default policy, this gives people a blank slate to work from.

Another option would have been to not infer a setting if someone sets a
handful of settings manually, which would have avoided the confusion in
Issue clap-rs#2599 but I see that being confusing (for someone who knows the
default, they will be expecting it to be additive; which flags?) and
brittle (as flags are added or changed, how do we ensure we keep this
up?)

Tests were added to ensure we support people customizing the behavior to
match their needs.

This is not solving:
- `Vec<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
epage added a commit to epage/clap that referenced this issue Nov 5, 2021
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 0, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple occurrences, optional
  - optional: `Vec` implies 0 or more, so should not imply required
- `Option<Vec<_>>`: multiple occurrences, optional
  - optional: Use over `Vec` to detect when no option being present when
    using multiple values

Motivations:

My priorities were:
1. Are we getting in the users way?
2. Does the API make sense?
3. Does the API encourage best practices?

I was originally concerned about the lack of composability with
`Option<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<Vec<_>>`).
It prescribes special meaning to each type depending on where it shows
up, rather than providing a single meaning for a type generally.  You
then can't do things like have `Option<_>` mean "required argument with
optional value" without hand constructing it.  However, in practice the
outer type correlates with the argument occurrence and the inner type with the
value.   It is rare to want the value behavior without also the
occurrence behavior.  So I figure it is probably fine as long as people
can set the flags to manually get the behavior they want.

`Vec<_>` implies multiple occurrences, rather than multiple values.
Anecdotally, whenever I've used the old `Arg::multiple`, I thought I was
getting `Arg::multiple_occurrences` only.  `Arg::multiple_values`,
without any bounds or delimiter requirement, can lead to a confusing
user experience and isn't a good default for these.  On top of that, if
someone does have an unbounded or a delimiter multiple values, they are
probably also using multiple occurrences.

`Vec<_>` is optional because a `Vec` implies 0 or more, so we stick to
the meaning of the rust type.  At least for me, I also rarely need a
required with multiple occurrences argument but more often need optional
with multiple occurrences.

`Option<Vec<_>>` ends up matching `Vec<_>` which an raise the question
of why have it.  Some users might prefer the type.  Otherwise, this is
so users can no when the argument is present or not when using
`min_values(0)`.  Rather than defining an entire policy around this and
having users customize it, or setting `min_values(0)` without the rest
of a default policy, this gives people a blank slate to work from.

Another option would have been to not infer a setting if someone sets a
handful of settings manually, which would have avoided the confusion in
Issue clap-rs#2599 but I see that being confusing (for someone who knows the
default, they will be expecting it to be additive; which flags?) and
brittle (as flags are added or changed, how do we ensure we keep this
up?)

Tests were added to ensure we support people customizing the behavior to
match their needs.

This is not solving:
- `Vec<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
epage added a commit to epage/clap that referenced this issue Nov 5, 2021
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 0, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple occurrences, optional
  - optional: `Vec` implies 0 or more, so should not imply required
- `Option<Vec<_>>`: multiple occurrences, optional
  - optional: Use over `Vec` to detect when no option being present when
    using multiple values

Motivations:

My priorities were:
1. Are we getting in the users way?
2. Does the API make sense?
3. Does the API encourage best practices?

I was originally concerned about the lack of composability with
`Option<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<Vec<_>>`).
It prescribes special meaning to each type depending on where it shows
up, rather than providing a single meaning for a type generally.  You
then can't do things like have `Option<_>` mean "required argument with
optional value" without hand constructing it.  However, in practice the
outer type correlates with the argument occurrence and the inner type with the
value.   It is rare to want the value behavior without also the
occurrence behavior.  So I figure it is probably fine as long as people
can set the flags to manually get the behavior they want.

`Vec<_>` implies multiple occurrences, rather than multiple values.
Anecdotally, whenever I've used the old `Arg::multiple`, I thought I was
getting `Arg::multiple_occurrences` only.  `Arg::multiple_values`,
without any bounds or delimiter requirement, can lead to a confusing
user experience and isn't a good default for these.  On top of that, if
someone does have an unbounded or a delimiter multiple values, they are
probably also using multiple occurrences.

`Vec<_>` is optional because a `Vec` implies 0 or more, so we stick to
the meaning of the rust type.  At least for me, I also rarely need a
required with multiple occurrences argument but more often need optional
with multiple occurrences.

`Option<Vec<_>>` ends up matching `Vec<_>` which can raise the question
of why have it.  Some users might prefer the type.  Otherwise, this is
so users can detect whether the argument is present or not when using
`min_values(0)`.  Rather than defining an entire policy around this and
having users customize it, or setting `min_values(0)` without the rest
of a default policy, this gives people a blank slate to work from.

Another design option would have been to not infer any special-type
settings if someone sets a handful of settings manually, which would
have avoided the confusion in Issue clap-rs#2599 but I see that being confusing
(for someone who knows the default, they will be expecting it to be
additive; which flags disable inferred settings?) and brittle (as flags
are added or changed, how do we ensure we keep this up?).

Tests were added to ensure we support people customizing the behavior to
match their needs.

This is not solving:
- `Vec<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717
- `Vec<Option<_>>` and many other potential combinations

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
@epage epage added A-parsing Area: Parser's logic and needs it changed somehow. S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed C: matches labels Dec 8, 2021
tmccombs added a commit to tmccombs/clap that referenced this issue Mar 10, 2022
tmccombs added a commit to tmccombs/clap that referenced this issue Mar 10, 2022
tmccombs added a commit to tmccombs/clap that referenced this issue Mar 10, 2022
@tmccombs
Copy link
Contributor

With the new action API, and typed arguments API, I wonder if the best way to expose this would be as a new ArgAction::GroupedAppend action, which is similar to the ArgAction::Append action, but, then the argument type changes from T to Vec<T>, and captures all arguments for each occurance/group. So then get_many::<Vec<T>>() would return a ValuesRef<'_, Vec<T>>.

Or perhaps a more general API could be to have a way to specify an additional parsing function, that takes an iterator over the arguments to a single occurrence/group and returns a new single value. I'm not sure exactly how the typing would work on that though, if you were to allow using this in addition to a value_parser.

@epage
Copy link
Member Author

epage commented Jun 26, 2022

Yes, ArgAction will hopefully help with this. I don't think we can reuse get_many for this as the value_parser setting the stored type happens at a different level than the processing of actions and so the underlying type cannot be modified. I suspect we'll need yet another set of functions. We'll need to distinguish their name from get_many / remove_many.

Depending on how this works out, my thought is to have ArgAction::Extend (current behavior) and ArgAction::Append (groups). I think the reason I used ArgAction::Append in v3.2 is because internally we are appending right now and if we add a new function it'll just expose that. We'd then just add Extend to not do that. The downside is that it will have a higher migration cost as people are using Append now when they mean Extend.

tmccombs added a commit to tmccombs/clap that referenced this issue Dec 6, 2022
Change it to be more consistent with get_one and get_many and related
functions.

Relates-To: clap-rs#2924
tmccombs added a commit to tmccombs/clap that referenced this issue Dec 9, 2022
@tmccombs
Copy link
Contributor

For using the new get/remove API, what should the names of the functions be. In #4544 I used get_groups, remove_groups, etc. However, I'm not sure that is the best name, and would be happy to change it to something else. Some other possible options I can think of:

  • get_many_grouped
  • get_grouped
  • get_grouped_values
  • get_as_groups
  • get_many_per_occurrence

I don't have a strong preference, among those, and maybe there is one I haven't thought of that would be better.

tmccombs added a commit to tmccombs/clap that referenced this issue Dec 10, 2022
Change it to be more consistent with get_one and get_many and related
functions.

Relates-To: clap-rs#2924
tmccombs added a commit to tmccombs/clap that referenced this issue Dec 10, 2022
Change it to be more consistent with get_one and get_many and related
functions.

Relates-To: clap-rs#2924
@epage
Copy link
Member Author

epage commented Dec 10, 2022

I don't remember where the original conversation happened but the problem is that group is overloaded with ArgGroup and can be confusing. We generally refer to each time an argument shows up as an "occurrence", so I think something like that in the name of each part of this feature would make sense.

For example, get_occurrences could possibly work now that we've removed occurrences_of. You had mentioned get_many_per_occurrence which I;m not as thrilled with but I am open to other ideas people have.

@tmccombs
Copy link
Contributor

get_occurences would be fine with me.

tmccombs added a commit to tmccombs/clap that referenced this issue Dec 12, 2022
Change it to be more consistent with get_one and get_many and related
functions.

Relates-To: clap-rs#2924
tmccombs added a commit to tmccombs/clap that referenced this issue Dec 14, 2022
Change it to be more consistent with get_one and get_many and related
functions.

Relates-To: clap-rs#2924
tmccombs added a commit to tmccombs/clap that referenced this issue Dec 16, 2022
Change it to be more consistent with get_one and get_many and related
functions.

Relates-To: clap-rs#2924
tmccombs added a commit to tmccombs/clap that referenced this issue Dec 20, 2022
Change it to be more consistent with get_one and get_many and related
functions.

Relates-To: clap-rs#2924
tmccombs added a commit to tmccombs/clap that referenced this issue Dec 20, 2022
Change it to be more consistent with get_one and get_many and related
functions.

Relates-To: clap-rs#2924
tmccombs added a commit to tmccombs/clap that referenced this issue Dec 21, 2022
Change it to be more consistent with get_one and get_many and related
functions.

Relates-To: clap-rs#2924
tmccombs added a commit to tmccombs/clap that referenced this issue Dec 21, 2022
tmccombs added a commit to tmccombs/clap that referenced this issue Jan 3, 2023
tmccombs added a commit to tmccombs/clap that referenced this issue Jan 3, 2023
tmccombs added a commit to tmccombs/clap that referenced this issue Jan 4, 2023
tmccombs added a commit to tmccombs/clap that referenced this issue Jan 8, 2023
@epage
Copy link
Member Author

epage commented Jan 10, 2023

@tmccombs any concerns with stablizing?

@tmccombs
Copy link
Contributor

Not from me

epage added a commit to epage/clap that referenced this issue Jan 13, 2023
This let's you get an arguments values, grouped by the occurrence of the
argument.

Fixes clap-rs#2924
epage added a commit to epage/clap that referenced this issue Jan 13, 2023
This let's you get an arguments values, grouped by the occurrence of the
argument.

Note: this does not stablize derive support.  That requires a blocking
change and can be enabled via `unstable-v5` flag.  See clap-rs#4626 for an
exploration of how we can make this easier in the future.

Fixes clap-rs#2924
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-tracking-issue Category: A tracking issue for an unstable feature S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants