From a85857dfb0546dd9d9f2198864b7363d7ca573d1 Mon Sep 17 00:00:00 2001 From: liudingming Date: Sat, 14 Aug 2021 15:50:39 +0800 Subject: [PATCH] Make overrides_with working when `MultipleValues` is enabled. --- src/build/arg/mod.rs | 54 ++++++++++++++++++++++----------------- src/parse/parser.rs | 4 +-- tests/posix_compatible.rs | 22 ++++++++++++++++ 3 files changed, 53 insertions(+), 27 deletions(-) diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs index d47d8fc006f..63a7d1d8ea5 100644 --- a/src/build/arg/mod.rs +++ b/src/build/arg/mod.rs @@ -1096,10 +1096,11 @@ impl<'help> Arg<'help> { /// **NOTE:** When an argument is overridden it is essentially as if it never was used, any /// conflicts, requirements, etc. are evaluated **after** all "overrides" have been removed /// - /// **WARNING:** Positional arguments and options which accept [`Multiple*`] cannot override - /// themselves (or we would never be able to advance to the next positional). If a positional - /// argument or option with one of the [`Multiple*`] settings lists itself as an override, it is - /// simply ignored. + /// **WARNING:** Positional arguments and options which accept + /// [`ArgSettings::MultipleOccurrences`] cannot override themselves (or we + /// would never be able to advance to the next positional). If a positional + /// argument or option with one of the [`ArgSettings::MultipleOccurrences`] + /// settings lists itself as an override, it is simply ignored. /// /// # Examples /// @@ -1136,8 +1137,10 @@ impl<'help> Arg<'help> { /// assert!(m.is_present("flag")); /// assert_eq!(m.occurrences_of("flag"), 1); /// ``` - /// Making an arg [`Multiple*`] and override itself is essentially meaningless. Therefore - /// clap ignores an override of self if it's a flag and it already accepts multiple occurrences. + /// + /// Making an arg [`ArgSettings::MultipleOccurrences`] and override itself + /// is essentially meaningless. Therefore clap ignores an override of self + /// if it's a flag and it already accepts multiple occurrences. /// /// ``` /// # use clap::{App, Arg}; @@ -1147,8 +1150,10 @@ impl<'help> Arg<'help> { /// assert!(m.is_present("flag")); /// assert_eq!(m.occurrences_of("flag"), 4); /// ``` - /// Now notice with options (which *do not* set one of the [`Multiple*`]), it's as if only the - /// last occurrence happened. + /// + /// Now notice with options (which *do not* set + /// [`ArgSettings::MultipleOccurrences`]), it's as if only the last + /// occurrence happened. /// /// ``` /// # use clap::{App, Arg}; @@ -1160,36 +1165,37 @@ impl<'help> Arg<'help> { /// assert_eq!(m.value_of("opt"), Some("other")); /// ``` /// - /// Just like flags, options with one of the [`Multiple*`] set, will ignore the "override self" - /// setting. + /// This will also work when [`ArgSettings::MultipleValues`] is enabled: /// /// ``` /// # use clap::{App, Arg}; /// let m = App::new("posix") - /// .arg(Arg::from("--opt [val]... 'some option'") - /// .overrides_with("opt")) - /// .get_matches_from(vec!["", "--opt", "first", "over", "--opt", "other", "val"]); + /// .arg( + /// Arg::new("opt") + /// .long("opt") + /// .takes_value(true) + /// .multiple_values(true) + /// .overrides_with("opt") + /// ) + /// .get_matches_from(vec!["", "--opt", "1", "2", "--opt", "3", "4", "5"]); /// assert!(m.is_present("opt")); - /// assert_eq!(m.occurrences_of("opt"), 2); - /// assert_eq!(m.values_of("opt").unwrap().collect::>(), &["first", "over", "other", "val"]); + /// assert_eq!(m.occurrences_of("opt"), 1); + /// assert_eq!(m.values_of("opt").unwrap().collect::>(), &["3", "4", "5"]); /// ``` /// - /// A safe thing to do if you'd like to support an option which supports multiple values, but - /// also is "overridable" by itself, is to not use [`UseValueDelimiter`] and *not* use - /// `MultipleValues` while telling users to separate values with a comma (i.e. `val1,val2`) + /// Just like flags, options with [`ArgSettings::MultipleOccurrences`] set + /// will ignore the "override self" setting. /// /// ``` /// # use clap::{App, Arg}; /// let m = App::new("posix") - /// .arg(Arg::from("--opt [val] 'some option'") + /// .arg(Arg::from("--opt [val]... 'some option'") /// .overrides_with("opt")) - /// .get_matches_from(vec!["", "--opt=some,other", "--opt=one,two"]); + /// .get_matches_from(vec!["", "--opt", "first", "over", "--opt", "other", "val"]); /// assert!(m.is_present("opt")); - /// assert_eq!(m.occurrences_of("opt"), 1); - /// assert_eq!(m.values_of("opt").unwrap().collect::>(), &["one,two"]); + /// assert_eq!(m.occurrences_of("opt"), 2); + /// assert_eq!(m.values_of("opt").unwrap().collect::>(), &["first", "over", "other", "val"]); /// ``` - /// [`Multiple*`]: crate::ArgSettings::MultipleValues - /// [`UseValueDelimiter`]: ArgSettings::UseValueDelimiter pub fn overrides_with(mut self, arg_id: T) -> Self { self.overrides.push(arg_id.into()); self diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 7b114f783c2..c087636ff69 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -1608,10 +1608,8 @@ impl<'help, 'app> Parser<'help, 'app> { } } // Only do self override for argument that is not positional - // argument or flag with one of the Multiple* setting - // enabled(which is a feature). + // argument or flag with MultipleOccurrences setting enabled. if (self.is_set(AS::AllArgsOverrideSelf) || override_self) - && !overrider.is_set(ArgSettings::MultipleValues) && !overrider.is_set(ArgSettings::MultipleOccurrences) && !overrider.is_positional() { diff --git a/tests/posix_compatible.rs b/tests/posix_compatible.rs index 744b9622f0c..8ba9e35b6ff 100644 --- a/tests/posix_compatible.rs +++ b/tests/posix_compatible.rs @@ -306,3 +306,25 @@ fn require_overridden_4() { let err = result.err().unwrap(); assert_eq!(err.kind, ErrorKind::MissingRequiredArgument); } + +#[test] +fn issue_1374_overrides_self_with_multiple_values() { + let app = App::new("test").arg( + Arg::new("input") + .long("input") + .takes_value(true) + .overrides_with("input") + .min_values(0), + ); + let m = app + .clone() + .get_matches_from(&["test", "--input", "a", "b", "c", "--input", "d"]); + assert_eq!(m.values_of("input").unwrap().collect::>(), &["d"]); + let m = app + .clone() + .get_matches_from(&["test", "--input", "a", "b", "--input", "c", "d"]); + assert_eq!( + m.values_of("input").unwrap().collect::>(), + &["c", "d"] + ); +}