Skip to content
This repository has been archived by the owner on Jan 1, 2022. It is now read-only.

Simplify the takes_value API (range-based takes_values) #198

Open
epage opened this issue Dec 6, 2021 · 0 comments
Open

Simplify the takes_value API (range-based takes_values) #198

epage opened this issue Dec 6, 2021 · 0 comments

Comments

@epage
Copy link
Owner

epage commented Dec 6, 2021

Issue by epage
Friday Aug 13, 2021 at 19:28 GMT
Originally opened as clap-rs/clap#2688


Discussed in clap-rs/clap#2676

Originally posted by epage August 10, 2021
Currently, we have

  • takes_value
  • multiple_values
  • min_values
  • max_values

We then have to clarify how these are related to each other, for example:

NOTE: This does not implicitly set Arg::multiple(true). This is because -o val -o val is multiple occurrences but a single value and -o val1 val2 is a single occurrence with multiple values. For positional arguments this does set Arg::multiple(true) because there is no way to determine the difference between multiple occurrences and multiple values.

(taken from docs.rs, since updated to say multiple_occurrences).

I had already been brainstorming on this idea when kbknapp mentioned

I think the crux of the problem is that we previously didn’t do a good job of articulating the difference between multiple_values, multiple_occurrences, and max_values_per_occurrence.

I think we can simplify this problem down to

  • takes_values(num: impl clap::IntoRange) with
    • impl IntoRange for usize
    • impl IntoRange for ...each range type

So you can do take_values(0), take_values(2), take_values(1..), take_values(0..=1)

  • Really, the motivation is to support all the various range types. Taking in a number is just a convenience while we are at it. I considered taking in a bool, but I think that is making it too magical and isn't as needed.
  • This was named based on Remove magic arg type in `Args` clap-rs/clap#2674 going through and takes_values(1) being the default.

While I was already talking with kbknapp, I brought this up and is thoughts were:

That is an interesting idea! It feels like it might incur some cognitive load to understand, although once you see it it makes total sense. I'd love to find a way to play with this idea without throwing it front and center and in the API all of a sudden.

Notes

For the question from kbknapp

For example, which makes more sense for a positional argument; multiple_values or multiple_occurrences? I think you and I would say multiple_values, but I can totally see how someone may try multiple_occurrences. This was papered over by having just multiple previously and things just working. However, we’ve seen that strategy didn’t work in the long run as it created more ambiguity than it solved.

I would say occurrences, actually. This would leave room for the two features to compose, like multiple_occurrences(true).takes_values(2). Not a common pattern but I think it speaks to an API when the features compose together in a natural way which this allows.

RE Delimiters

I would posit that common cases for multiple values are

  • 0..=1
  • 2
  • 0.. or 1.. with a delimiter

We should raise awareness of require_delimiter in the docs for take_values.

Benefits

  • Clarifies our messaging on these settings plus occurrences since the focus is on fixed number of values or delimiters
  • Reduces the number of interrelated settings that a user has to understand (and we have to document in hopes it helps the user understand)
  • Uses higher level Rust types rather than a bool and numbers just hanging around (which ties into the Rust API guidelines)
  • I suspect this will also help us towards a more maintainable value/occurrence/inferring behavior.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant