Skip to content

Commit

Permalink
fix(derive): Define multiple policy for Special Types
Browse files Browse the repository at this point in the history
Before:
- `bool`: a flag
- `Option<_>`: not required
- `Option<Option<_>>` is not required and when it is present, the value
  is not required
- `Vec<_>`: multiple values, optional
- `Option<Vec<_>>`: multiple values, min values of 0, optional

After:
- `bool`: a flag
- `Option<_>`: not required
- `Option<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<Vec<_>>`: 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<Option<_>>` and `Option<Vec<_>>` (and eventually `Vec<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<Vec<_>>` 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 clap-rs#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<Vec<_>>`, see clap-rs#2924
- `(T1, T2)`, `Vec<(T1, T2)>`, etc, see clap-rs#1717

Fixes clap-rs#1772
Fixes clap-rs#2599
See also clap-rs#2195
  • Loading branch information
epage committed Nov 5, 2021
1 parent b9d007d commit 5f85704
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 14 deletions.
6 changes: 2 additions & 4 deletions clap_derive/src/derives/args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -313,7 +312,6 @@ pub fn gen_augment(

Ty::Other if flag => quote_spanned! { ty.span()=>
.takes_value(false)
.multiple_values(false)
},

Ty::Other => {
Expand Down
8 changes: 2 additions & 6 deletions clap_derive/tests/arg_enum.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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());
}
Expand All @@ -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])
Expand All @@ -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());
}
66 changes: 62 additions & 4 deletions clap_derive/tests/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand All @@ -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<i32>,
}
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<i32>,
}
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()
Expand Down Expand Up @@ -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<Vec<i32>>,
}
assert_eq!(
Opt { arg: Some(vec![1]) },
Opt::try_parse_from(&["test", "-a", "1"]).unwrap()
);

assert_eq!(
Opt {
arg: Some(vec![1, 2])
Expand Down Expand Up @@ -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!(
Expand All @@ -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!(
Expand Down

0 comments on commit 5f85704

Please sign in to comment.