Merge pull request clap-rs#1164 from kbknapp/v2.29.3

V2.29.3

CHANGELOG.md

@@ -1,3 +1,22 @@
+<a name="v2.29.3"></a>
+### v2.29.3 (2018-02-05)
+
+
+#### Improvements
+
+* **Overrides:**  clap now supports arguments which override with themselves ([6c7a0010](https://github.com/kbknapp/clap-rs/commit/6c7a001023ca1eac1cc6ffe6c936b4c4a2aa3c45), closes [#976](https://github.com/kbknapp/clap-rs/issues/976))
+
+#### Bug Fixes
+
+* **Requirements:**  fixes an issue where conflicting args would still show up as required ([e06cefac](https://github.com/kbknapp/clap-rs/commit/e06cefac97083838c0a4e1444dcad02a5c3f911e), closes [#1158](https://github.com/kbknapp/clap-rs/issues/1158))
+* Fixes a bug which disallows proper nesting of `--` ([73993fe](https://github.com/kbknapp/clap-rs/commit/73993fe30d135f682e763ec93dcb0814ed518011), closes [#1161](https://github.com/kbknapp/clap-rs/issues/1161))
+
+#### New Settings
+
+* **AllArgsOverrideSelf:**  adds a new convenience setting to allow all args to override themselves ([4670325d](https://github.com/kbknapp/clap-rs/commit/4670325d1bf0369addec2ae2bcb56f1be054c924))
+
+
+
 <a name="v2.29.2"></a>
 ### v2.29.2 (2018-01-16)
 

Cargo.toml

@@ -1,7 +1,7 @@
 [package]
 
 name = "clap"
-version = "2.29.2"
+version = "2.29.3"
 authors = ["Kevin K. <kbknapp@gmail.com>"]
 exclude = ["examples/*", "clap-test/*", "tests/*", "benches/*", "*.png", "clap-perf/*", "*.dot"]
 repository = "https://github.com/kbknapp/clap-rs"

README.md

@@ -42,25 +42,32 @@ Created by [gh-md-toc](https://github.com/ekalinin/github-markdown-toc)
 
 ## What's New
 
+Here's whats new in 2.29.3:
+
+* **Self Overrides:** now supports arguments which override themselves (Allows true shell aliases, config files, etc!)
+* **Requirements:**  fixes an issue where conflicting args would still show up as required and missing
+* Fixes a bug which disallows proper nesting of `--` for args that capture all values after the first `--`
+* **AppSettings::AllArgsOverrideSelf:**  adds a new convenience setting to allow all args to override themselves
+
 Here's whats new in 2.29.2:
 
 * **Many ZSH Completions Improvements** (Thanks to @segevfiner)
   *  Positional arguments will default to file completion when not using specific values!
-  *  Implement postional argument possible values completion 
-  *  Removes redundant code from output 
-  *  Don't pass `-S` to `_arguments` if Zsh is too old 
-  *  Fix completions with mixed positionals and subcommands 
-  *  String escape possible values for options 
+  *  Implement postional argument possible values completion
+  *  Removes redundant code from output
+  *  Don't pass `-S` to `_arguments` if Zsh is too old
+  *  Fix completions with mixed positionals and subcommands
+  *  String escape possible values for options
 
 
 Here's whats new in 2.29.1:
 
 * Debloats clap by deduplicating logic and refactors for a ~57% decrease in code size! This is with zero functinoality lost, and a slight perf increase!
-* Change the bash completion script code generation to support hyphens. 
+* Change the bash completion script code generation to support hyphens.
 * Fix completion of long option values in ZSH completions
 * Fixes broken links in docs
 * Updates contributors list
-* Fixes the ripgrep benchmark by adding a value to a flag that expects it 
+* Fixes the ripgrep benchmark by adding a value to a flag that expects it
 
 Here's whats new in 2.29.0:
 

src/app/parser.rs

@@ -251,6 +251,7 @@ where
     fn add_reqs(&mut self, a: &Arg<'a, 'b>) {
         if a.is_set(ArgSettings::Required) {
             // If the arg is required, add all it's requirements to master required list
+            self.required.push(a.b.name);
             if let Some(ref areqs) = a.b.requires {
                 for name in areqs
                     .iter()
@@ -260,7 +261,6 @@ where
                     self.required.push(name);
                 }
             }
-            self.required.push(a.b.name);
         }
     }
 
@@ -909,9 +909,15 @@ where
                 }
 
                 if starts_new_arg {
+                    let check_all = self.is_set(AS::AllArgsOverrideSelf);
                     {
                         let any_arg = find_any_by_name!(self, self.cache.unwrap_or(""));
-                        matcher.process_arg_overrides(any_arg, &mut self.overrides, &mut self.required);
+                        matcher.process_arg_overrides(
+                            any_arg, 
+                            &mut self.overrides, 
+                            &mut self.required, 
+                            check_all
+                        );
                     }
 
                     if arg_os.starts_with(b"--") {
@@ -1039,9 +1045,15 @@ where
                     self.settings.set(AS::TrailingValues);
                 }
                 if self.cache.map_or(true, |name| name != p.b.name) {
+                    let check_all = self.is_set(AS::AllArgsOverrideSelf);
                     {
                         let any_arg = find_any_by_name!(self, self.cache.unwrap_or(""));
-                        matcher.process_arg_overrides(any_arg, &mut self.overrides, &mut self.required);
+                        matcher.process_arg_overrides(
+                            any_arg, 
+                            &mut self.overrides, 
+                            &mut self.required, 
+                            check_all
+                        );
                     }
                     self.cache = Some(p.b.name);
                 }
@@ -1157,9 +1169,15 @@ where
         }
 
         // In case the last arg was new, we  need to process it's overrides
+        let check_all = self.is_set(AS::AllArgsOverrideSelf);
         {
             let any_arg = find_any_by_name!(self, self.cache.unwrap_or(""));
-            matcher.process_arg_overrides(any_arg, &mut self.overrides, &mut self.required);
+            matcher.process_arg_overrides(
+                any_arg, 
+                &mut self.overrides, 
+                &mut self.required, 
+                check_all
+            );
         }
 
         self.remove_overrides(matcher);

src/app/settings.rs

@@ -46,6 +46,7 @@ bitflags! {
         const VALID_ARG_FOUND      = 1 << 37;
         const INFER_SUBCOMMANDS    = 1 << 38;
         const CONTAINS_LAST        = 1 << 39;
+        const ARGS_OVERRIDE_SELF   = 1 << 40;
     }
 }
 
@@ -75,6 +76,7 @@ impl AppFlags {
     impl_settings! { AppSettings,
         ArgRequiredElseHelp => Flags::A_REQUIRED_ELSE_HELP,
         ArgsNegateSubcommands => Flags::ARGS_NEGATE_SCS,
+        AllArgsOverrideSelf => Flags::ARGS_OVERRIDE_SELF,
         AllowExternalSubcommands => Flags::ALLOW_UNK_SC,
         AllowInvalidUtf8 => Flags::UTF8_NONE,
         AllowLeadingHyphen => Flags::LEADING_HYPHEN,
@@ -165,6 +167,13 @@ pub enum AppSettings {
     /// [`ArgMatches::lossy_values_of`]: ./struct.ArgMatches.html#method.lossy_values_of
     AllowInvalidUtf8,
 
+    /// Essentially sets [`Arg::overrides_with("itself")`] for all arguments.
+    ///
+    /// **WARNING:** Positional arguments cannot override themselves (or we would never be able
+    /// to advance to the next positional). This setting ignores positional arguments.
+    /// [`Arg::overrides_with("itself")`]: ./struct.Arg.html#method.overrides_with
+    AllArgsOverrideSelf,
+
     /// Specifies that leading hyphens are allowed in argument *values*, such as negative numbers
     /// like `-10`. (which would otherwise be parsed as another flag or option)
     ///

src/app/validator.rs

@@ -36,11 +36,14 @@ impl<'a, 'b, 'z> Validator<'a, 'b, 'z> {
         self.0.add_defaults(matcher)?;
         if let ParseResult::Opt(a) = needs_val_of {
             debugln!("Validator::validate: needs_val_of={:?}", a);
-            let o = self.0
+            let o = {
+                self.0
                 .opts
                 .iter()
                 .find(|o| o.b.name == a)
-                .expect(INTERNAL_ERROR_MSG);
+                .expect(INTERNAL_ERROR_MSG)
+                .clone()
+            };
             self.validate_required(matcher)?;
             reqs_validated = true;
             let should_err = if let Some(v) = matcher.0.args.get(&*o.b.name) {
@@ -50,7 +53,7 @@ impl<'a, 'b, 'z> Validator<'a, 'b, 'z> {
             };
             if should_err {
                 return Err(Error::empty_value(
-                    o,
+                    &o,
                     &*usage::create_error_usage(self.0, matcher, None),
                     self.0.color(),
                 ));
@@ -336,7 +339,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) {
@@ -438,20 +441,46 @@ impl<'a, 'b, 'z> Validator<'a, 'b, 'z> {
         Ok(())
     }
 
-    fn validate_required(&self, matcher: &ArgMatcher) -> ClapResult<()> {
+    fn validate_required(&mut self, matcher: &ArgMatcher) -> ClapResult<()> {
         debugln!(
             "Validator::validate_required: required={:?};",
             self.0.required
         );
 
-        'outer: for name in &self.0.required {
+        let mut should_err = false;
+        let mut to_rem = Vec::new();
+        for name in &self.0.required {
             debugln!("Validator::validate_required:iter:{}:", name);
             if matcher.contains(name) {
-                continue 'outer;
+                continue;
             }
-            if let Some(a) = find_any_by_name!(self.0, *name) {
+            if to_rem.contains(name) {
+                continue;
+            } else if let Some(a) = find_any_by_name!(self.0, *name) {
                 if self.is_missing_required_ok(a, matcher) {
-                    continue 'outer;
+                    to_rem.push(a.name());
+                    if let Some(reqs) = a.requires() {
+                        for r in reqs
+                            .iter()
+                            .filter(|&&(val, _)| val.is_none())
+                            .map(|&(_, name)| name)
+                        {
+                            to_rem.push(r);
+                        }
+                    }
+                    continue;
+                }
+            }
+            should_err = true;
+            break;
+        }
+        if should_err {
+            for r in &to_rem {
+                'inner: for i in (0 .. self.0.required.len()).rev() {
+                    if &self.0.required[i] == r {
+                        self.0.required.swap_remove(i);
+                        break 'inner;
+                    }
                 }
             }
             return self.missing_required_error(matcher, None);

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,74 @@ 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 (essentially
+    /// preventing a "Unexpected multiple usage" error):
+    ///
+    /// ```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. Therefore
+    /// clap ignores an override of self if it's a flag and it already accepts multiple occurrences.
+    ///
+    /// ```
+    /// # 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"), 4);
+    /// ```
+    /// Now notice 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::<Vec<_>>(), &["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::<Vec<_>>(), &["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());