Stablize `ArgMatches::grouped_value_of` Tracking Issue
epage opened this issue · 10 comments
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?
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?
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.
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.
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.
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_groupedget_groupedget_grouped_valuesget_as_groupsget_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.
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.
get_occurences would be fine with me.
Not from me