From 5f857043727dfaa77e2f458172e1b69eb0b6d05c Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 27 Oct 2021 13:33:36 -0500 Subject: [PATCH] fix(derive): Define multiple policy for Special Types Before: - `bool`: a flag - `Option<_>`: not required - `Option>` is not required and when it is present, the value is not required - `Vec<_>`: multiple values, optional - `Option>`: multiple values, min values of 0, optional After: - `bool`: a flag - `Option<_>`: not required - `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>`: 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>` and `Option>` (and eventually `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>` 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 #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>`, see #2924 - `(T1, T2)`, `Vec<(T1, T2)>`, etc, see #1717 Fixes #1772 Fixes #2599 See also #2195 --- clap_derive/src/derives/args.rs | 6 +-- clap_derive/tests/arg_enum.rs | 8 +--- clap_derive/tests/options.rs | 66 +++++++++++++++++++++++++++++++-- 3 files changed, 66 insertions(+), 14 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 6f7a0e6e6990..ae5466b65eb3 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -289,8 +289,7 @@ pub fn gen_augment( Ty::OptionVec => quote_spanned! { ty.span()=> .takes_value(true) .value_name(#value_name) - .multiple_values(true) - .min_values(0) + .multiple_occurrences(true) #possible_values #validator #allow_invalid_utf8 @@ -300,7 +299,7 @@ pub fn gen_augment( quote_spanned! { ty.span()=> .takes_value(true) .value_name(#value_name) - .multiple_values(true) + .multiple_occurrences(true) #possible_values #validator #allow_invalid_utf8 @@ -313,7 +312,6 @@ pub fn gen_augment( Ty::Other if flag => quote_spanned! { ty.span()=> .takes_value(false) - .multiple_values(false) }, Ty::Other => { diff --git a/clap_derive/tests/arg_enum.rs b/clap_derive/tests/arg_enum.rs index 809734ed8140..1eb64bfca577 100644 --- a/clap_derive/tests/arg_enum.rs +++ b/clap_derive/tests/arg_enum.rs @@ -419,7 +419,7 @@ fn vec_type() { Opt { arg: vec![ArgChoice::Foo, ArgChoice::Bar] }, - Opt::try_parse_from(&["", "-a", "foo", "bar"]).unwrap() + Opt::try_parse_from(&["", "-a", "foo", "-a", "bar"]).unwrap() ); assert!(Opt::try_parse_from(&["", "-a", "fOo"]).is_err()); } @@ -439,10 +439,6 @@ fn option_vec_type() { } assert_eq!(Opt { arg: None }, Opt::try_parse_from(&[""]).unwrap()); - assert_eq!( - Opt { arg: Some(vec![]) }, - Opt::try_parse_from(&["", "-a"]).unwrap() - ); assert_eq!( Opt { arg: Some(vec![ArgChoice::Foo]) @@ -453,7 +449,7 @@ fn option_vec_type() { Opt { arg: Some(vec![ArgChoice::Foo, ArgChoice::Bar]) }, - Opt::try_parse_from(&["", "-a", "foo", "bar"]).unwrap() + Opt::try_parse_from(&["", "-a", "foo", "-a", "bar"]).unwrap() ); assert!(Opt::try_parse_from(&["", "-a", "fOo"]).is_err()); } diff --git a/clap_derive/tests/options.rs b/clap_derive/tests/options.rs index 3ee40d2c82ca..43fece81845e 100644 --- a/clap_derive/tests/options.rs +++ b/clap_derive/tests/options.rs @@ -259,7 +259,7 @@ fn two_option_option_types() { } #[test] -fn vec_type_is_multiple_values() { +fn vec_type_is_multiple_occurrences() { #[derive(Parser, PartialEq, Debug)] struct Opt { #[clap(short, long)] @@ -270,6 +270,42 @@ fn vec_type_is_multiple_values() { Opt::try_parse_from(&["test", "-a24"]).unwrap() ); assert_eq!(Opt { arg: vec![] }, Opt::try_parse_from(&["test"]).unwrap()); + assert_eq!( + Opt { arg: vec![24, 42] }, + Opt::try_parse_from(&["test", "-a", "24", "-a", "42"]).unwrap() + ); +} + +#[test] +fn vec_type_with_required() { + #[derive(Parser, PartialEq, Debug)] + struct Opt { + #[clap(short, long, required = true)] + arg: Vec, + } + assert_eq!( + Opt { arg: vec![24] }, + Opt::try_parse_from(&["test", "-a24"]).unwrap() + ); + assert!(Opt::try_parse_from(&["test"]).is_err()); + assert_eq!( + Opt { arg: vec![24, 42] }, + Opt::try_parse_from(&["test", "-a", "24", "-a", "42"]).unwrap() + ); +} + +#[test] +fn vec_type_with_multiple_values_only() { + #[derive(Parser, PartialEq, Debug)] + struct Opt { + #[clap(short, long, multiple_values(true), multiple_occurrences(false))] + arg: Vec, + } + assert_eq!( + Opt { arg: vec![24] }, + Opt::try_parse_from(&["test", "-a24"]).unwrap() + ); + assert_eq!(Opt { arg: vec![] }, Opt::try_parse_from(&["test"]).unwrap()); assert_eq!( Opt { arg: vec![24, 42] }, Opt::try_parse_from(&["test", "-a", "24", "42"]).unwrap() @@ -308,6 +344,28 @@ fn option_vec_type() { Opt::try_parse_from(&["test", "-a", "1"]).unwrap() ); + assert_eq!( + Opt { + arg: Some(vec![1, 2]) + }, + Opt::try_parse_from(&["test", "-a", "1", "-a", "2"]).unwrap() + ); + + assert_eq!(Opt { arg: None }, Opt::try_parse_from(&["test"]).unwrap()); +} + +#[test] +fn option_vec_type_structopt_behavior() { + #[derive(Parser, PartialEq, Debug)] + struct Opt { + #[clap(short, long, multiple_values(true), min_values(0))] + arg: Option>, + } + assert_eq!( + Opt { arg: Some(vec![1]) }, + Opt::try_parse_from(&["test", "-a", "1"]).unwrap() + ); + assert_eq!( Opt { arg: Some(vec![1, 2]) @@ -337,9 +395,9 @@ fn two_option_vec_types() { assert_eq!( Opt { arg: Some(vec![1]), - b: Some(vec![]) + b: None, }, - Opt::try_parse_from(&["test", "-a", "1", "-b"]).unwrap() + Opt::try_parse_from(&["test", "-a", "1"]).unwrap() ); assert_eq!( @@ -355,7 +413,7 @@ fn two_option_vec_types() { arg: Some(vec![1, 2]), b: Some(vec![1, 2]) }, - Opt::try_parse_from(&["test", "-a", "1", "2", "-b", "1", "2"]).unwrap() + Opt::try_parse_from(&["test", "-a", "1", "-a", "2", "-b", "1", "-b", "2"]).unwrap() ); assert_eq!(