Remove magic arg type in `Args`

epage commented 2 years ago

Issue by epage Friday Aug 13, 2021 at 19:27 GMT Originally opened as https://github.com/clap-rs/clap/issues/2687

Discussed in https://github.com/clap-rs/clap/discussions/2674

^{Originally posted by **epage** August 9, 2021} - `Arg::new()` creates a positional argument that `takes_value(true)` - `Args::new().long()` creates a named flag (`takes_value(false)`) - `Args::new().long().takes_value(true)` is required to created a named arg / option This requires us to guess the users intent at various stages. kbknapp explained this when trying to explain existing challenges with `values` vs `occurrences`: > Keeping in mind that clap has some implied rules that at times contradict one-another in subtle ways. For example Arg::new effectively creates a positional value, i.e. an argument that accepts a single value with flags/switches. Adding either short or long actually removes the value, and turns it into a flag (i.e. switch with no associated value). Then adding takes_value (or any of the value associated methods) implicitly turns it into an option (combined flag/switch with an associated value), so giving it a value again. > > Because of how these rules/settings combine, it turns out there are multiple ways to end up at the same final product; some just more explicit while others passively allow the rules to combine. As in the Arg::new example above, that's all that's required to create a positional argument, but doing Arg::new("foo").takes_value(true).index(1) works just as well. > > Because of this, clap sometimes has to infer if the user meant to allow the rules to combine. This primarily comes out around the multiple values/occurrences. In other words clap has a default state for many of the settings, and we need to ensure that when a user only sets one (or a few) settings the remaining defaults don't combine to do something surprising. I'm even running into a problem I'm debugging for #751 that I think comes from this. Just a small tweak would make clap less brittle for change and I believe make the user experience better, if we switch to - `Arg::new()` creates a positional argument that `takes_value(true)` - `Args::new().long()` creates a named arg / option (ie `takes_value(true)`) - `Args::new().long().takes_value(false)` is required to created a flag @kbknapp was open to changing this, though with a different approach (this was before clarifying the approach above, see below for his thoughts on each approach) > I’m not against removing changing the default behavior of Arg::new, the only reason it hasn’t been done before is the only other sensible thing to do (IMO) is to have Arg::new result in a “useless” argument, Further methods would be required to determine this. This would accomplish what you’re speaking about, and solve a few of the issues related to ambiguities. The only reason I didn’t do that before, was I tend to lean towards a contractor should at least make the struct usable to some extent…but I can see how in this case that may not be the case. We could fix the "always valid object" problem by instead making some helper constructors (`positional`, `flag`, and `option` where the latter take long names since 99% of the time, they are used) Between flag-opt-in and construct invalid options (I didn't bring up the named constructors), kbknapp said > I think it's almost 50/50 as to which route would be preferred. Opt-in to a flag (takes_value(false)) I think really only has the downside of being different from v2 causing (minimal) churn in that those that currently have Arg::new().long() would auto-magically have all their flags turn into options unless they go in and add takes_value(false) to them all. Whereas the I believe the number of users who are using Arg::new and no other "value based" methods and just relying on it being a positional are few and far between, so less churn. But again, I'm somewhat 50/50 as to which I think is "better" so to speak.

epage commented 2 years ago

Comment by epage Friday Aug 13, 2021 at 20:52 GMT

Note to self: when implementing this, look into also implementing #2688

epage commented 2 years ago

Comment by kbknapp Saturday Aug 14, 2021 at 00:53 GMT

The idea of using helper constructors positional, flag, or option is a great idea IMO instead of Arg::new resulting in a not "always valid object." In fact I've seen several binaries do almost exactly this with type aliases and macros, ripgrep being the largest (at least they did this a while back when I looked...not sure about the current status).

epage commented 2 years ago

Comment by pksunkara Saturday Aug 14, 2021 at 00:54 GMT

From the discussion,

If we want takes_value to be true by default, Arg::new would have takes_value(true) set, which means it is a valid positional. Which is why I was saying it will be always valid and we don't need helper constructors

epage commented 2 years ago

Comment by epage Saturday Aug 14, 2021 at 00:55 GMT

Yeah, I got the idea from ripgrep which I copied until I switched to Structopt. I did a quick look earlier as part of this conversation and it looks like burntsushi has moved away from it.

epage commented 2 years ago

Comment by kbknapp Saturday Aug 14, 2021 at 01:18 GMT

@pksunkara I was thinking more about the long game of leave Arg::new as it is for v3, but deprecate it and introduce three constructors that are more explicit about what they're doing. Or least discuss doing so, even if it's not until v4.

Just for backstory; I think the point I was trying to make in the thread between @epage and I was that currently since Arg::new returns a valid positional argument, when one simply adds short or long to it, clap implicitly unsets takes_value(true) turning the argument into a flag which is somewhat surprising to the user (at least until they learn that's how it works). Now it's not that big a deal, and I doubt this particular issue has caused anyone true confusion.

But this minor constructor problem is indicative of certain other similar cases in clap that can/do either cause confusion for the consumer, or ambiguities for clap parsing itself. Ambiguities aren't the worst thing, but they lead to implicit parsing rules, that if not specified somewhere can cause subtle breakage or cause unintended consequences as the API grows or evolves.

clap's original design(tm) was deliberately different from something like getopt which had distinct argument types (i.e. at the type system level). I found that most often I would iterate over my CLI design and change between various arg types fluidly while coming up with something that felt right for the particular tool I was working on. APIs like getopt made that iteration harder because changing an arguments type was more involved. Whereas in clap it usually meant either adding or removing a single method call and that's it. Now as the clap API grew this caused some minor issues, like why does a "flag" effectively have all the methods for handling values? But it had a huge benefit that if I wanted to turn my flag into an option which I just added a single method call for that nifty new thing I wanted to try and done. Didn't work and back to a flag? Just comment out that single line again. It also had the benefit of being able to treat positional values and values of options as identical, which most of the time is nice (but it's not without oddities). Yes, that can be done through traits as well, but then you either end up with massive code bloat via generics along with the pain that threading these generics through the call stacks can be, or the slower dynamic dispatch.

Hopefully that makes sense, or at least captures some the "why" from the early days :smile:

epage commented 2 years ago

Comment by epage Tuesday Aug 17, 2021 at 16:41 GMT

Thanks for the background!

Unless someone has an alternative idea on how things might work, I don't think Arg::new can ever go away.

API options

Name-only

fn positional(name: &str);
fn flag(name: &str);
fn option(name: &str);

Can get rid of Arg::new
But, doesn't enforce flag and option are "valid", short or long is still needed
We could infer long from name if no flag is no long/short is set. Might confuse users when adding a short removes the long
We could infer long from name generally. This turns it into an implicit version of the "Helper" solution below and needs Arg::new

Option-all-the-way

fn postional(name: &str);
fn flag(name: &str, long: Option<&str>, short: Option<char>);
fn option(name: &str: long: Option<&str>, short: Option<char>);

This is not much different than the "Name-only" option since someone can do None, None
Feels a bit ugly to me

Helper

fn new(name: &str);
fn postional(name: &str);
fn flag(name: &str, long: &str);
fn option(name: &str: long: &str);

flag and option are always valid but you can't have a short-only flag/option
Arg::new needed for the 1% case of short-only
Can still construct into an invalid state

epage / clapng