Skip to content

Commit

Permalink
fix(parser): Conflict-with-self is back on by default
Browse files Browse the repository at this point in the history
See clap-rs#4261 for more details
  • Loading branch information
epage committed Sep 26, 2022
1 parent 4094807 commit 46c076a
Show file tree
Hide file tree
Showing 16 changed files with 91 additions and 63 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
Expand Up @@ -183,6 +183,9 @@ Subtle changes (i.e. compiler won't catch):
- `ArgAction::Count`, requiring `ArgMatches::get_count` instead of `ArgMatches::occurrences_of`
- `ArgAction::Set`, requiring `ArgMatches::get_one` instead of `ArgMatches::value_of`
- `ArgAction::Append`, requiring `ArgMatches::get_many` instead of `ArgMatches::values_of`
- `ArgAction::Set`, `ArgAction::SetTrue`, and `Arg::Action::SetFalse` now
conflict by default to be like `ArgAction::StoreValue` and
`ArgAction::IncOccurrences`, requiring `cmd.args_override_self(true)` to override instead (#4261)
- By default, an `Arg`s default action is `ArgAction::Set`, rather than `ArgAction::IncOccurrence` to reduce confusing magic through consistency (#2687, #4032, see also #3977)
- `mut_arg` can no longer be used to customize help and version arguments, instead disable them (`Command::disable_help_flag`, `Command::disable_version_flag`) and provide your own (#4056)
- Removed lifetimes from `Command`, `Arg`, `ArgGroup`, and `PossibleValue`, assuming `'static`. `string` feature flag will enable support for `String`s (#1041, #2150, #4223)
Expand Down
7 changes: 6 additions & 1 deletion examples/tutorial_builder/03_01_flag_bool.md
Expand Up @@ -16,6 +16,11 @@ $ 03_01_flag_bool --verbose
verbose: true

$ 03_01_flag_bool --verbose --verbose
verbose: true
? failed
error: The argument '--verbose' cannot be used with '--verbose'

Usage: 03_01_flag_bool[EXE] [OPTIONS]

For more information try '--help'

```
7 changes: 6 additions & 1 deletion examples/tutorial_derive/03_01_flag_bool.md
Expand Up @@ -16,6 +16,11 @@ $ 03_01_flag_bool_derive --verbose
verbose: true

$ 03_01_flag_bool_derive --verbose --verbose
verbose: true
? failed
error: The argument '--verbose' cannot be used with '--verbose'

Usage: 03_01_flag_bool_derive[EXE] [OPTIONS]

For more information try '--help'

```
16 changes: 14 additions & 2 deletions src/builder/action.rs
Expand Up @@ -27,6 +27,10 @@
pub enum ArgAction {
/// When encountered, store the associated value(s) in [`ArgMatches`][crate::ArgMatches]
///
/// **NOTE:** If the argument has previously been seen, it will result in a
/// [`ArgumentConflict`][crate::error::ErrorKind::ArgumentConflict] unless
/// [`Command::args_override_self(true)`][crate::Command::args_override_self] is set.
///
/// # Examples
///
/// ```rust
Expand Down Expand Up @@ -76,6 +80,10 @@ pub enum ArgAction {
/// No value is allowed. To optionally accept a value, see
/// [`Arg::default_missing_value`][super::Arg::default_missing_value]
///
/// **NOTE:** If the argument has previously been seen, it will result in a
/// [`ArgumentConflict`][crate::error::ErrorKind::ArgumentConflict] unless
/// [`Command::args_override_self(true)`][crate::Command::args_override_self] is set.
///
/// # Examples
///
/// ```rust
Expand All @@ -88,7 +96,7 @@ pub enum ArgAction {
/// .action(clap::ArgAction::SetTrue)
/// );
///
/// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
/// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag"]).unwrap();
/// assert!(matches.contains_id("flag"));
/// assert_eq!(
/// matches.get_one::<bool>("flag").copied(),
Expand Down Expand Up @@ -123,7 +131,7 @@ pub enum ArgAction {
/// )
/// );
///
/// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
/// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag"]).unwrap();
/// assert!(matches.contains_id("flag"));
/// assert_eq!(
/// matches.get_one::<usize>("flag").copied(),
Expand All @@ -145,6 +153,10 @@ pub enum ArgAction {
/// No value is allowed. To optionally accept a value, see
/// [`Arg::default_missing_value`][super::Arg::default_missing_value]
///
/// **NOTE:** If the argument has previously been seen, it will result in a
/// [`ArgumentConflict`][crate::error::ErrorKind::ArgumentConflict] unless
/// [`Command::args_override_self(true)`][crate::Command::args_override_self] is set.
///
/// # Examples
///
/// ```rust
Expand Down
2 changes: 1 addition & 1 deletion src/builder/value_parser.rs
Expand Up @@ -657,7 +657,7 @@ pub trait TypedValueParser: Clone + Send + Sync + 'static {
/// )
/// );
///
/// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
/// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag"]).unwrap();
/// assert!(matches.contains_id("flag"));
/// assert_eq!(
/// matches.get_one::<usize>("flag").copied(),
Expand Down
2 changes: 1 addition & 1 deletion src/parser/matches/arg_matches.rs
Expand Up @@ -162,7 +162,7 @@ impl ArgMatches {
/// .action(clap::ArgAction::SetTrue)
/// );
///
/// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag", "--flag"]).unwrap();
/// let matches = cmd.clone().try_get_matches_from(["mycmd", "--flag"]).unwrap();
/// assert!(matches.contains_id("flag"));
/// assert_eq!(
/// matches.get_flag("flag"),
Expand Down
2 changes: 1 addition & 1 deletion src/parser/parser.rs
Expand Up @@ -1239,7 +1239,7 @@ impl<'cmd> Parser<'cmd> {
raw_vals
};

if matcher.remove(&arg.id) && self.cmd.is_args_conflicts_with_self() {
if matcher.remove(&arg.id) && self.cmd.is_args_override_self() {
return Err(ClapError::argument_conflict(
self.cmd,
arg.to_string(),
Expand Down
15 changes: 15 additions & 0 deletions tests/builder/action.rs
@@ -1,6 +1,7 @@
#![allow(clippy::bool_assert_comparison)]

use clap::builder::ArgPredicate;
use clap::error::ErrorKind;
use clap::Arg;
use clap::ArgAction;
use clap::Command;
Expand All @@ -22,8 +23,15 @@ fn set() {
assert_eq!(matches.contains_id("mammal"), true);
assert_eq!(matches.index_of("mammal"), Some(2));

let result = cmd
.clone()
.try_get_matches_from(["test", "--mammal", "dog", "--mammal", "cat"]);
let err = result.err().unwrap();
assert_eq!(err.kind(), ErrorKind::ArgumentConflict);

let matches = cmd
.clone()
.args_override_self(true)
.try_get_matches_from(["test", "--mammal", "dog", "--mammal", "cat"])
.unwrap();
assert_eq!(matches.get_one::<String>("mammal").unwrap(), "cat");
Expand Down Expand Up @@ -88,8 +96,15 @@ fn set_true() {
assert_eq!(matches.contains_id("mammal"), true);
assert_eq!(matches.index_of("mammal"), Some(1));

let result = cmd
.clone()
.try_get_matches_from(["test", "--mammal", "--mammal"]);
let err = result.err().unwrap();
assert_eq!(err.kind(), ErrorKind::ArgumentConflict);

let matches = cmd
.clone()
.args_override_self(true)
.try_get_matches_from(["test", "--mammal", "--mammal"])
.unwrap();
assert_eq!(*matches.get_one::<bool>("mammal").unwrap(), true);
Expand Down
60 changes: 5 additions & 55 deletions tests/builder/app_settings.rs
Expand Up @@ -986,65 +986,11 @@ fn built_in_subcommand_escaped() {
}
}

#[test]
fn aaos_flags() {
// flags
let cmd = Command::new("posix").arg(arg!(--flag "some flag").action(ArgAction::SetTrue));

let res = cmd.clone().try_get_matches_from(vec!["", "--flag"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(m.contains_id("flag"));
assert!(*m.get_one::<bool>("flag").expect("defaulted by clap"));

let res = cmd.try_get_matches_from(vec!["", "--flag", "--flag", "--flag", "--flag"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(m.contains_id("flag"));
assert!(*m.get_one::<bool>("flag").expect("defaulted by clap"));
}

#[test]
fn aaos_flags_mult() {
// flags with multiple
let cmd = Command::new("posix").arg(arg!(--flag "some flag").action(ArgAction::Count));

let res = cmd.clone().try_get_matches_from(vec!["", "--flag"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(m.contains_id("flag"));
assert_eq!(*m.get_one::<u8>("flag").expect("defaulted by clap"), 1);

let res = cmd.try_get_matches_from(vec!["", "--flag", "--flag", "--flag", "--flag"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(m.contains_id("flag"));
assert_eq!(*m.get_one::<u8>("flag").expect("defaulted by clap"), 4);
}

#[test]
fn aaos_opts() {
// opts
let res = Command::new("posix")
.arg(
arg!(--opt <val> "some option")
.required(true)
.action(ArgAction::Set),
)
.try_get_matches_from(vec!["", "--opt=some", "--opt=other"]);
assert!(res.is_ok(), "{}", res.unwrap_err());
let m = res.unwrap();
assert!(m.contains_id("opt"));
assert_eq!(
m.get_one::<String>("opt").map(|v| v.as_str()),
Some("other")
);
}

#[test]
fn aaos_opts_w_other_overrides() {
// opts with other overrides
let res = Command::new("posix")
.args_override_self(true)
.arg(arg!(--opt <val> "some option").action(ArgAction::Set))
.arg(
arg!(--other <val> "some other option")
Expand All @@ -1066,6 +1012,7 @@ fn aaos_opts_w_other_overrides() {
fn aaos_opts_w_other_overrides_rev() {
// opts with other overrides, rev
let res = Command::new("posix")
.args_override_self(true)
.arg(
arg!(--opt <val> "some option")
.required(true)
Expand All @@ -1092,6 +1039,7 @@ fn aaos_opts_w_other_overrides_rev() {
fn aaos_opts_w_other_overrides_2() {
// opts with other overrides
let res = Command::new("posix")
.args_override_self(true)
.arg(
arg!(--opt <val> "some option")
.overrides_with("other")
Expand All @@ -1113,6 +1061,7 @@ fn aaos_opts_w_other_overrides_2() {
fn aaos_opts_w_other_overrides_rev_2() {
// opts with other overrides, rev
let res = Command::new("posix")
.args_override_self(true)
.arg(
arg!(--opt <val> "some option")
.required(true)
Expand Down Expand Up @@ -1259,6 +1208,7 @@ fn aaos_pos_mult() {
#[test]
fn aaos_option_use_delim_false() {
let m = Command::new("posix")
.args_override_self(true)
.arg(
arg!(--opt <val> "some option")
.required(true)
Expand Down
2 changes: 2 additions & 0 deletions tests/builder/flags.rs
Expand Up @@ -19,6 +19,7 @@ fn flag_using_short() {
#[test]
fn lots_o_flags_sep() {
let r = Command::new("opts")
.args_override_self(true)
.arg(arg!(o: -o ... "some flag").action(ArgAction::SetTrue))
.try_get_matches_from(vec![
"", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o", "-o",
Expand Down Expand Up @@ -53,6 +54,7 @@ fn lots_o_flags_sep() {
#[test]
fn lots_o_flags_combined() {
let r = Command::new("opts")
.args_override_self(true)
.arg(arg!(o: -o ... "some flag").action(ArgAction::SetTrue))
.try_get_matches_from(vec![
"",
Expand Down
1 change: 1 addition & 0 deletions tests/builder/grouped_values.rs
Expand Up @@ -223,6 +223,7 @@ fn grouped_interleaved_positional_occurrences() {
#[test]
fn issue_2171() {
let schema = Command::new("ripgrep#1701 reproducer")
.args_override_self(true)
.arg(
Arg::new("pretty")
.short('p')
Expand Down
11 changes: 11 additions & 0 deletions tests/builder/indices.rs
Expand Up @@ -3,6 +3,7 @@ use clap::{Arg, ArgAction, Command};
#[test]
fn indices_mult_opts() {
let m = Command::new("ind")
.args_override_self(true)
.arg(
Arg::new("exclude")
.short('e')
Expand Down Expand Up @@ -32,6 +33,7 @@ fn indices_mult_opts() {
#[test]
fn index_mult_opts() {
let m = Command::new("ind")
.args_override_self(true)
.arg(
Arg::new("exclude")
.short('e')
Expand All @@ -55,6 +57,7 @@ fn index_mult_opts() {
#[test]
fn index_flag() {
let m = Command::new("ind")
.args_override_self(true)
.arg(Arg::new("exclude").short('e').action(ArgAction::SetTrue))
.arg(Arg::new("include").short('i').action(ArgAction::SetTrue))
.try_get_matches_from(vec!["ind", "-e", "-i"])
Expand All @@ -67,6 +70,7 @@ fn index_flag() {
#[test]
fn index_flags() {
let m = Command::new("ind")
.args_override_self(true)
.arg(Arg::new("exclude").short('e').action(ArgAction::SetTrue))
.arg(Arg::new("include").short('i').action(ArgAction::SetTrue))
.try_get_matches_from(vec!["ind", "-e", "-i", "-e", "-e", "-i"])
Expand All @@ -79,6 +83,7 @@ fn index_flags() {
#[test]
fn indices_mult_flags() {
let m = Command::new("ind")
.args_override_self(true)
.arg(Arg::new("exclude").short('e').action(ArgAction::SetTrue))
.arg(Arg::new("include").short('i').action(ArgAction::SetTrue))
.try_get_matches_from(vec!["ind", "-e", "-i", "-e", "-e", "-i"])
Expand All @@ -91,6 +96,7 @@ fn indices_mult_flags() {
#[test]
fn indices_mult_flags_combined() {
let m = Command::new("ind")
.args_override_self(true)
.arg(Arg::new("exclude").short('e').action(ArgAction::SetTrue))
.arg(Arg::new("include").short('i').action(ArgAction::SetTrue))
.try_get_matches_from(vec!["ind", "-eieei"])
Expand All @@ -103,6 +109,7 @@ fn indices_mult_flags_combined() {
#[test]
fn indices_mult_flags_opt_combined() {
let m = Command::new("ind")
.args_override_self(true)
.arg(Arg::new("exclude").short('e').action(ArgAction::SetTrue))
.arg(Arg::new("include").short('i').action(ArgAction::SetTrue))
.arg(Arg::new("option").short('o').action(ArgAction::Set))
Expand All @@ -117,6 +124,7 @@ fn indices_mult_flags_opt_combined() {
#[test]
fn indices_mult_flags_opt_combined_eq() {
let m = Command::new("ind")
.args_override_self(true)
.arg(Arg::new("exclude").short('e').action(ArgAction::SetTrue))
.arg(Arg::new("include").short('i').action(ArgAction::SetTrue))
.arg(Arg::new("option").short('o').action(ArgAction::Set))
Expand All @@ -131,6 +139,7 @@ fn indices_mult_flags_opt_combined_eq() {
#[test]
fn indices_mult_opt_value_delim_eq() {
let m = Command::new("myapp")
.args_override_self(true)
.arg(
Arg::new("option")
.short('o')
Expand All @@ -149,6 +158,7 @@ fn indices_mult_opt_value_delim_eq() {
#[test]
fn indices_mult_opt_value_no_delim_eq() {
let m = Command::new("myapp")
.args_override_self(true)
.arg(
Arg::new("option")
.short('o')
Expand All @@ -163,6 +173,7 @@ fn indices_mult_opt_value_no_delim_eq() {
#[test]
fn indices_mult_opt_mult_flag() {
let m = Command::new("myapp")
.args_override_self(true)
.arg(Arg::new("option").short('o').action(ArgAction::Append))
.arg(Arg::new("flag").short('f').action(ArgAction::SetTrue))
.try_get_matches_from(vec!["myapp", "-o", "val1", "-f", "-o", "val2", "-f"])
Expand Down
2 changes: 2 additions & 0 deletions tests/builder/multiple_occurrences.rs
Expand Up @@ -3,6 +3,7 @@ use clap::{arg, Arg, ArgAction, Command};
#[test]
fn multiple_occurrences_of_flags_long() {
let m = Command::new("mo_flags_long")
.args_override_self(true)
.arg(arg!(--multflag "allowed multiple flag").action(ArgAction::SetTrue))
.arg(arg!(--flag "disallowed multiple flag").action(ArgAction::SetTrue))
.try_get_matches_from(vec!["", "--multflag", "--flag", "--multflag"])
Expand All @@ -16,6 +17,7 @@ fn multiple_occurrences_of_flags_long() {
#[test]
fn multiple_occurrences_of_flags_short() {
let m = Command::new("mo_flags_short")
.args_override_self(true)
.arg(arg!(-m --multflag "allowed multiple flag").action(ArgAction::SetTrue))
.arg(arg!(-f --flag "disallowed multiple flag").action(ArgAction::SetTrue))
.try_get_matches_from(vec!["", "-m", "-f", "-m"])
Expand Down
2 changes: 1 addition & 1 deletion tests/builder/multiple_values.rs
Expand Up @@ -401,7 +401,7 @@ fn option_max_more() {

#[test]
fn optional_value() {
let mut cmd = Command::new("test").arg(
let mut cmd = Command::new("test").args_override_self(true).arg(
Arg::new("port")
.short('p')
.value_name("NUM")
Expand Down
1 change: 1 addition & 0 deletions tests/derive/flags.rs
Expand Up @@ -21,6 +21,7 @@ use clap::Parser;
#[test]
fn bool_type_is_flag() {
#[derive(Parser, PartialEq, Eq, Debug)]
#[command(args_override_self = true)]
struct Opt {
#[arg(short, long)]
alice: bool,
Expand Down

0 comments on commit 46c076a

Please sign in to comment.