diff --git a/src/app/validator.rs b/src/app/validator.rs index 9751321a1490..665ebb09cf74 100644 --- a/src/app/validator.rs +++ b/src/app/validator.rs @@ -336,7 +336,7 @@ impl<'a, 'b, 'z> Validator<'a, 'b, 'z> { where A: AnyArg<'a, 'b> + Display, { - debugln!("Validator::validate_arg_num_vals;"); + debugln!("Validator::validate_arg_num_vals:{}", a.name()); if let Some(num) = a.num_vals() { debugln!("Validator::validate_arg_num_vals: num_vals set...{}", num); let should_err = if a.is_set(ArgSettings::Multiple) { diff --git a/src/args/arg.rs b/src/args/arg.rs index bdcd63a5ec43..108530c58245 100644 --- a/src/args/arg.rs +++ b/src/args/arg.rs @@ -1203,6 +1203,10 @@ impl<'a, 'b> Arg<'a, 'b> { /// **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 cannot override themselves (or we would never be able + /// to advance to the next positional). If a positional agument lists itself as an override, + /// it is simply ignored. + /// /// # Examples /// /// ```rust @@ -1222,6 +1226,72 @@ impl<'a, 'b> Arg<'a, 'b> { /// // was never used because it was overridden with color /// assert!(!m.is_present("flag")); /// ``` + /// Care must be taken when using this setting, and having an arg override with itself. This + /// is common practice when supporting things like shell aliases, config files, etc. + /// However, when combined with multiple values, it can get dicy. + /// Here is how clap handles such situations: + /// + /// When a flag overrides itself, it's as if the flag was only ever used once: + /// + /// ```rust + /// # use clap::{App, Arg}; + /// let m = App::new("posix") + /// .arg(Arg::from_usage("--flag 'some flag'").overrides_with("flag")) + /// .get_matches_from(vec!["posix", "--flag", "--flag"]); + /// assert!(m.is_present("flag")); + /// assert_eq!(m.occurrences_of("flag"), 1); + /// ``` + /// Making a flag `multiple(true)` and override itself is essentially meaningless. + /// + /// ``` + /// # use clap::{App, Arg}; + /// let m = App::new("posix") + /// .arg(Arg::from_usage("--flag... 'some flag'").overrides_with("flag")) + /// .get_matches_from(vec!["", "--flag", "--flag", "--flag", "--flag"]); + /// assert!(m.is_present("flag")); + /// assert_eq!(m.occurrences_of("flag"), 1); + /// ``` + /// Now notice the same thing happens with options, it's as if only the last occurrence mattered + /// + /// ``` + /// # use clap::{App, Arg}; + /// let m = App::new("posix") + /// .arg(Arg::from_usage("--opt [val] 'some option'").overrides_with("opt")) + /// .get_matches_from(vec!["", "--opt=some", "--opt=other"]); + /// assert!(m.is_present("opt")); + /// assert_eq!(m.occurrences_of("opt"), 1); + /// assert_eq!(m.value_of("opt"), Some("other")); + /// ``` + /// + /// Here is where it gets interesting. If an option is declared as `multiple(true)` and it also + /// overrides with itself, only the last *set* of values will be saved. + /// + /// ``` + /// # use clap::{App, Arg}; + /// let m = App::new("posix") + /// .arg(Arg::from_usage("--opt [val]... 'some option'") + /// .overrides_with("opt")) + /// .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::>(), &["other", "val"]); + /// ``` + /// + /// A safe thing to do, to ensure there is no confusion is to require an argument delimiter and + /// and only one "value set" per instance of the option. + /// + /// ``` + /// # use clap::{App, Arg}; + /// let m = App::new("posix") + /// .arg(Arg::from_usage("--opt [val]... 'some option'") + /// .overrides_with("opt") + /// .number_of_values(1) + /// .require_delimiter(true)) + /// .get_matches_from(vec!["", "--opt=some,other", "--opt=one,two"]); + /// assert!(m.is_present("opt")); + /// assert_eq!(m.occurrences_of("opt"), 1); + /// assert_eq!(m.values_of("opt").unwrap().collect::>(), &["one", "two"]); + /// ``` pub fn overrides_with(mut self, name: &'a str) -> Self { if let Some(ref mut vec) = self.b.overrides { vec.push(name.as_ref()); diff --git a/src/args/arg_matcher.rs b/src/args/arg_matcher.rs index 25f3fc5d749a..12df293a8e48 100644 --- a/src/args/arg_matcher.rs +++ b/src/args/arg_matcher.rs @@ -21,13 +21,17 @@ impl<'a> Default for ArgMatcher<'a> { impl<'a> ArgMatcher<'a> { pub fn new() -> Self { ArgMatcher::default() } - pub fn process_arg_overrides<'b>(&mut self, a: Option<&AnyArg<'a, 'b>>, overrides: &mut Vec<(&'b str, &'a str)>, required: &mut Vec<&'a str>) { + pub fn process_arg_overrides<'b>(&mut self, a: Option<&AnyArg<'a, 'b>>, overrides: &mut Vec<(&'b str, &'a str)>, required: &mut Vec<&'a str>, check_all: bool) { debugln!("ArgMatcher::process_arg_overrides:{:?};", a.map_or(None, |a| Some(a.name()))); if let Some(aa) = a { + let mut self_done = false; if let Some(a_overrides) = aa.overrides() { for overr in a_overrides { debugln!("ArgMatcher::process_arg_overrides:iter:{};", overr); - if self.is_present(overr) { + if overr == &aa.name() { + self_done = true; + self.handle_self_overrides(a); + } else if self.is_present(overr) { debugln!("ArgMatcher::process_arg_overrides:iter:{}: removing from matches;", overr); self.remove(overr); for i in (0 .. required.len()).rev() { @@ -37,11 +41,35 @@ impl<'a> ArgMatcher<'a> { break; } } + overrides.push((overr, aa.name())); } else { overrides.push((overr, aa.name())); } } } + if check_all && !self_done { + self.handle_self_overrides(a); + } + } + } + + pub fn handle_self_overrides<'b>(&mut self, a: Option<&AnyArg<'a, 'b>>) { + debugln!("ArgMatcher::handle_self_overrides:{:?};", a.map_or(None, |a| Some(a.name()))); + if let Some(aa) = a { + if !aa.has_switch() { + // positional args can't override self or else we would never advance to the next + return; + } + if let Some(ma) = self.get_mut(aa.name()) { + if ma.vals.len() > 1 { + // swap_remove(0) would be O(1) but does not preserve order, which + // we need + ma.vals.remove(0); + ma.occurs = 1; + } else if !aa.takes_value() && ma.occurs > 1 { + ma.occurs = 1; + } + } } } @@ -143,7 +171,6 @@ impl<'a> ArgMatcher<'a> { occurs: 0, vals: Vec::with_capacity(1), }); - // let len = ma.vals.len() + 1; ma.vals.push(val.to_owned()); }