-
Notifications
You must be signed in to change notification settings - Fork 0
clap_derive: Vec/Option<Vec> behavior is inconsistent with other types #148
Comments
Comment by TeXitoi
I strongly disagree on this. A vector can have 0 elements, and that's the most used case. If you need at least one, you can add |
Comment by CreepySkeleton If we use
|
Comment by pksunkara The
Can you explain what this means? I don't understand the sentence. I think we should have |
Comment by TeXitoi I really disagree. Most of the time you're using a Vec, you just want the list of parameters, and you don't really care if an empty parameter was provided. Forcing using an option of vec in the most common case would impact a lot the ergonomics. |
Comment by cecton Here is a use case where I find structopt very confusing: paritytech/substrate@6b0eed4 When I read the struct I see that listen_addr is a required argument and port is optional. But the structopt parameters shows me that both arguments are conflicting. Before that commit, the parameters weren't declared as conflicting and I had no way to know that listen_addr was actually optional. The only way I could know that is by reading the table. I personally do think that I invite you to read the lengthy discussion here.
Maybe but it will be consistent with the rest of the API. |
Comment by CreepySkeleton Pff, it took me some time to get back to, sorry for the delays guys.
That issue is related here indeed, but we can't leave it out of this discussion because this is about defaults in derive, not about distinguishing between the two.
@TeXitoi I see your point about But we wouldn't be forcing anything! Users would still be very much able to do struct Opts {
// This Vec is not option so it's required BY DEFAULT
// because it's not bool and not Option
required_vec: Vec<u32>,
// We can override default behavior
// while keeping the raw Vec
#[clap(required = false)]
not_required_vec: Vec<u32>,
// We can use Option<Vec> instead of overriding
// We lose some of the ergonomics but acquire the
// ability to distinguish between <empty list> an <no option at all>
opt_vec: Option<Vec<u32>>
} In other words, this is question of good defaults, and I believe I've found the answer. I think that our willingness of changing defaults should depend of what the most widespread usage is in practice. I decided to check via grep.app and this has been eye opening for me: (IMPORTANT: this research assumes that
attributes with regexes, speak up!)
|
Comment by pksunkara
I don't agree with this. Maybe all of them tried Also, we are not technically breaking this because this a separate library
Note:
By default, if one wants to support a
|
Comment by CreepySkeleton
Sounds very unlikely, but maybe. @TeXitoi , has anybody ever told you something like "I tried
I agree, absolutely. But we aren't talking just about breaking changes, we're talking about good defaults and whether it's worth to change them. If we find that the current defaults are not optimal, we shall change them, otherwise we shall leave them as is.
Eleven ( I like your
This is exactly what |
Comment by CreepySkeleton
This part of reply ended up being literally drenched in sarcasm, an therefore was moved to it's own comment. I'm not blaming anyone and neither I mean to offend anybody. Just stating that the state of affairs is ridiculous. Sure thing. According to 2.x docs. Well, let's test it! Playground extern crate clap;
use clap::{App, Arg};
fn main() {
let m = App::new("app")
.arg(Arg::with_name("arg").multiple(true).takes_value(true))
.arg(Arg::with_name("opt").long("foo").multiple(true).takes_value(true))
.get_matches_from(&["test", "val1", "val2", "--foo", "optv1", "optv2"]);
println!("{:?}", m.values_of("arg").unwrap().collect::<Vec<_>>());
println!("{:?}", m.values_of("opt").unwrap().collect::<Vec<_>>());
} So, we expect it to error, right?
Wait, what? In practice, I've seen a lot of people taking Well, the master is much better! Right? Right. You'd expect it to be documented properly...
Of course. "May appear only once". Crystal clear. What the function looks like?
Sarcasm aside, I very much like how it's done in master:
Well done. Well done indeed. No sarcasm, the API is clear and tidy. Except copy-pasting docs and examples from 2.x probably wasn't the brightest idea. |
Comment by TeXitoi
I've never seen any question about |
Comment by CreepySkeleton Also, the last like of @pksunkara 's table is impossible unless user explicitly specifies
|
Comment by pksunkara In that case, assume that I meant What do you mean by |
Comment by CreepySkeleton
In clap 2: it does so explicitly, this is documented:
In clap 3: not explicitly, I haven't been able to locate the place this is handled in, but I just checked this program: use clap::{App, Arg};
fn main() {
let m = App::new("app")
.arg(Arg::with_name("opt").long("foo").multiple_values(true))
.get_matches();
println!("{:?}", m.values_of("opt").map(|v| v.collect::<Vec<_>>()));
} And yes, it works just like if
|
Comment by CreepySkeleton Actually, if you wrap your head around the terminology a bit, it becomes quite clear:
It's a problem of good documentation more than anything, about the way it conveys this setup to user. I'm amending my previous statement about |
Comment by kbknapp Whew ok that was a read! Ever time I see issues pop up around Then clap 2 came around and there was confusion when people wanted multiple values, but not multiple occurrences, or vice versa. It was also over-loaded with flags which only occurrences are possible. This spawned things like Looking back, I wish I'd made the sane default of I also wanted to use the 3.x breaking changes to fix this confusion once and for all. I have a much better idea of what is most common. I want the defaults to be the most common case, and allow those who need something different be able to do so. Unfortunately before I went on hiatus I didn't finish the docs which I'm sure has lead to even more confusion around the various types of multiples, and their defaults or interactions with each other. I see multiples as one of those things, "It sounds sooo simple at first, why can't it just work!?" Until you dig into the details and realize, "Ooooh. Yeah I can totally see how some would want X while others need Y, and you need to distinguish between them sometimes, but not always." It's kind of like Strings in that manner; such a simple concept yet so hard to do correctly. ...Or CLIs for that matter 😜 Ok, so back to the topic at hand: I find myself agreeing with @CreepySkeleton here. We're talking about sane defaults. I can also totally respect @TeXitoi's sentiment of So I think this comes down to balancing Rust ergonomics and semantics ( I would suggest simply calling it out in the docs directly, that although the default is Also, perhaps I'm out of touch, but if I wanted to allow multiple values with occurrences via derive in clap I'd instinctivily try to do
Correct. That is intended behaviour due to how parsing is structured. I don't think there is anything we can, or would want to do about this.
100% agree. Having said all that, if we settled on |
Comment by sourcefrog I just came here from TeXitoi/structopt#396 and wanted to say, I really hope in a v3 breaking change, Clap can address the
+100 this bit. It really sticks out as a trap in what's generally a friendly API, and is exacerbated by the fact that you can easily not notice the bug is there. |
Comment by epage Catching up on this issue. Sorry if I missed something or am just repeating others. Multiple values: From a code author and user perspective, I have found variable number of values ( Delimited values: When I want to support multiple values, I tend to do it with a delimiter ( When I'm writing a CLI, I tend to prefer multiple occurrences with delimited values all flattened, There tends to be some friction in this process, so I tend to only do it when I really need it. When I do skip on one, I skip on delimited values, preferring multiple occurrences. It tends to be more composable. If I need require at least one value, it is across both occurrences and delimited values.
So far, the talk seems to be about Something being left out is alternative mental models, like focusing on the type's semantics, for which So let's break down some type semantics:
(we don't have a native type for "1 or more" ( And with the notes at the top, our considerations are:
My proposal would be to match the type semantics, focusing on occurrences:
Note: one downside to implementing any intuitive behavior is people have to predict both what was intuitive to you and how complete your implementation is. No-intuitive behavior is fully explicit and predictable. A theoretical fully-intuitive system would be, by definition, predictable. When you land in the middle, people feel some uncertainty. How quickly that uncertainty is dispelled is proportionate to the quality of your library(1) in framing (API design, docs explaining) (2) making it quick to answer questions (reference). As an aside, I think |
Comment by pksunkara Tuples and other adapters can be handled/discussed in clap-rs/clap#1717. What we need to finalize in this issue are the |
Comment by epage To clarify, my intent was not to pull #1717 into this but to look at the problem holistically to see how it can affect decisions made here. |
Comment by pksunkara I think we need to first decide on a table like https://docs.rs/structopt/0.3.22/structopt/#type-magic |
Comment by epage My hope is to look at the problem holistically, defining principles for us to Background
structopt:
Definitions:
clap_derive post-3.0.0-beta.2
Definitions
History
Potential ConsiderationsBrevity: what gets the shortest code Obvious code: is the intent clear? Obvious API: can I predict how the API will behave (i.e. how much do I need docs?)?
Pit of Success: is the path of least resistance following best practices? Are problematic practices discouraged through resistance? Features: Are all supported features possible to use? ToolboxTypes
Features
clap_derive (proposed)
Notes
Motivation:
Potential future improvement: I have some hand-wayy ideas for how we can improve the |
Comment by pksunkara Ignoring the tuples part of the discussion for now.
I am afraid I don't understand what you meant here.
I don't agree. While vec does convey that it can be empty, that's not exactly same semantics here. Here, when you mean optional, the semantic is whether vec should be present in the first place. |
Comment by epage
I'm not quite following this logic. |
Comment by pksunkara
Yes, it conveys that it can be empty. But that's the semantics for the values of the arg. It doesn't say anything about whether the arg is required or optional. ( |
Comment by pksunkara
I agree, but that is what I want us to resolve here. Provide easy way to do |
Comment by epage Since I found some of the original structopt PRs, thought I'd note them here in case we can mine useful historical information |
Comment by epage Before:
After:
Motivations: My priorities were:
I was originally concerned about the lack of composability with
Another design option would have been to not infer any special-type Tests were added to ensure we support people customizing the behavior to For now, I am leaving off:
To see this in action, see clap-rs/clap#2993 |
Issue by CreepySkeleton
Tuesday Mar 31, 2020 at 12:47 GMT
Originally opened as clap-rs/clap#1772
This is actually a summary of TeXitoi/structopt#364
Current behavior
Vec<T>
andOption<Vec<T>>
are notrequired = true
by default.Vec<T>
ismultiple = true
by default which allows not only multiple values (--foo 1 2 3
) but also multiple occurrences (--foo 1 --foo 2 3
).Option<Vec<T>>
additionally allows zero number of values (--foo
).What's wrong
Vec<T>
is notrequired
by default is inconsistent with all the other types inclap_derive
that are required by default unless they are wrapped inOption
(exceptbool
but it's a very special case).Option<Vec<T>>
is different fromVec<T>
, different not in "not required" sense, confuses newcomers.Vec<T>
allows multiple occurrences along with values is misleading.Proposal
min_values = 1
for bothOption<Vec<T>>
andVec<T>
instead ofmultiple = true
, allowing only non-zero number of values and disallow multiple occurrences (--foo 1 2
but not--foo
nor--foo 1 --foo 2
). If a user wants to allow zero values or multiple occurrences as well, they can explicitly specify it viamin_values = 0
andmultiple = true
respectively.required = true
forVec<T>
.cc @TeXitoi @Dylan-DPC @pksunkara
The text was updated successfully, but these errors were encountered: