Auto merge of #898 - kbknapp:issues-896,895, r=kbknapp

Issues 896,895

CHANGELOG.md

@@ -1,3 +1,14 @@
+<a name="v2.21.1"></a>
+### v2.21.1 (2017-03-12)
+
+
+#### Bug Fixes
+
+* **ArgRequiredElseHelp:**  fixes the precedence of this error to prioritize over other error messages ([74b751ff](https://github.com/kbknapp/clap-rs/commit/74b751ff2e3631e337b7946347c1119829a41c53), closes [#895](https://github.com/kbknapp/clap-rs/issues/895))
+* **Positionals:**  fixes some regression bugs resulting from old asserts in debug mode. ([9a3bc98e](https://github.com/kbknapp/clap-rs/commit/9a3bc98e9b55e7514b74b73374c5ac8b6e5e0508), closes [#896](https://github.com/kbknapp/clap-rs/issues/896))
+
+
+
 <a name="v2.21.0"></a>
 ## v2.21.0 (2017-03-09)
 

Cargo.toml

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

README.md

@@ -45,6 +45,11 @@ Created by [gh-md-toc](https://github.com/ekalinin/github-markdown-toc)
 
 ## What's New
 
+Here's the highlights for v2.21.1
+
+* fixes the precedence of this error to prioritize over other error messages
+* fixes some regression bugs resulting from old asserts in debug mode.
+
 Here's the highlights for v2.21.0
 
 * adds the ability to mark a positional argument as 'last' which means it should be used with `--` syntax and can be accessed early to effectivly skip other positional args

src/app/parser.rs

@@ -455,31 +455,30 @@ impl<'a, 'b> Parser<'a, 'b>
             // Or the second to last has a terminator or .last(true) set
             let ok = last.is_set(ArgSettings::Required) ||
                      (second_to_last.v.terminator.is_some() ||
-                      second_to_last.b.is_set(ArgSettings::Last));
+                      second_to_last.b.is_set(ArgSettings::Last)) ||
+                     last.is_set(ArgSettings::Last);
             assert!(ok,
                     "When using a positional argument with .multiple(true) that is *not the \
                           last* positional argument, the last positional argument (i.e the one \
                           with the highest index) *must* have .required(true) or .last(true) set.");
-            let num = self.positionals.len() - 1;
-            let ok = self.positionals
-                .get(num)
-                .unwrap()
-                .is_set(ArgSettings::Multiple);
+            let ok = second_to_last.is_set(ArgSettings::Multiple) || last.is_set(ArgSettings::Last);
             assert!(ok,
                     "Only the last positional argument, or second to last positional \
                           argument may be set to .multiple(true)");
 
-            self.settings.set(AS::LowIndexMultiplePositional);
+            let count = self.positionals
+                .values()
+                .filter(|p| p.b.settings.is_set(ArgSettings::Multiple) && p.v.num_vals.is_none())
+                .count();
+            let ok = count <= 1 ||
+                     (last.is_set(ArgSettings::Last) && last.is_set(ArgSettings::Multiple) &&
+                      second_to_last.is_set(ArgSettings::Multiple) &&
+                      count == 2);
+            assert!(ok,
+                    "Only one positional argument with .multiple(true) set is allowed per \
+                        command, unless the second one also has .last(true) set");
         }
 
-        let ok = self.positionals
-            .values()
-            .filter(|p| p.b.settings.is_set(ArgSettings::Multiple) && p.v.num_vals.is_none())
-            .map(|_| 1)
-            .sum::<u64>() <= 1;
-        assert!(ok,
-                "Only one positional argument with .multiple(true) set is allowed per \
-                      command");
 
         if self.is_set(AS::AllowMissingPositional) {
             // Check that if a required positional argument is found, all positions with a lower
@@ -677,7 +676,6 @@ impl<'a, 'b> Parser<'a, 'b>
 
     // allow wrong self convention due to self.valid_neg_num = true and it's a private method
     #[cfg_attr(feature = "lints", allow(wrong_self_convention))]
-    #[inline]
     fn is_new_arg(&mut self, arg_os: &OsStr, needs_val_of: Option<&'a str>) -> bool {
         debugln!("Parser::is_new_arg: arg={:?}, Needs Val of={:?}",
                  arg_os,
@@ -743,6 +741,13 @@ impl<'a, 'b> Parser<'a, 'b>
         debugln!("Parser::get_matches_with;");
         // Verify all positional assertions pass
         debug_assert!(self.verify_positionals());
+        if self.positionals.values().any(|a| {
+                                             a.b.is_set(ArgSettings::Multiple) &&
+                                             (a.index as usize != self.positionals.len())
+                                         }) &&
+           self.positionals.values().last().map_or(false, |p| !p.is_set(ArgSettings::Last)) {
+            self.settings.set(AS::LowIndexMultiplePositional);
+        }
         let has_args = self.has_args();
 
         // Next we create the `--help` and `--version` arguments and add them if
@@ -761,6 +766,11 @@ impl<'a, 'b> Parser<'a, 'b>
             self.unset(AS::ValidNegNumFound);
             // Is this a new argument, or values from a previous option?
             let starts_new_arg = self.is_new_arg(&arg_os, needs_val_of);
+            if arg_os.starts_with(b"--") && arg_os.len_() == 2 {
+                debugln!("Parser::get_matches_with: setting TrailingVals=true");
+                self.set(AS::TrailingValues);
+                continue;
+            }
 
             // Has the user already passed '--'? Meaning only positional args follow
             if !self.is_set(AS::TrailingValues) {
@@ -792,15 +802,6 @@ impl<'a, 'b> Parser<'a, 'b>
                     }
                 } else {
                     if arg_os.starts_with(b"--") {
-                        if arg_os.len_() == 2 {
-                            // The user has passed '--' which means only positional args follow no
-                            // matter what they start with
-                            debugln!("Parser::get_matches_with: found '--'");
-                            debugln!("Parser::get_matches_with: setting TrailingVals=true");
-                            self.set(AS::TrailingValues);
-                            continue;
-                        }
-
                         needs_val_of = try!(self.parse_long_arg(matcher, &arg_os));
                         if !(needs_val_of.is_none() && self.is_set(AS::AllowLeadingHyphen)) {
                             continue;

src/app/validator.rs

@@ -49,13 +49,6 @@ impl<'a, 'b, 'z> Validator<'a, 'b, 'z> {
             }
         }
 
-        try!(self.validate_blacklist(matcher));
-        if !(self.0.is_set(AS::SubcommandsNegateReqs) && subcmd_name.is_some()) && !reqs_validated {
-            try!(self.validate_required(matcher));
-        }
-        try!(self.validate_matched_args(matcher));
-        matcher.usage(usage::create_usage_with_title(self.0, &[]));
-
         if matcher.is_empty() && matcher.subcommand_name().is_none() &&
            self.0.is_set(AS::ArgRequiredElseHelp) {
             let mut out = vec![];
@@ -66,6 +59,13 @@ impl<'a, 'b, 'z> Validator<'a, 'b, 'z> {
                            info: None,
                        });
         }
+        try!(self.validate_blacklist(matcher));
+        if !(self.0.is_set(AS::SubcommandsNegateReqs) && subcmd_name.is_some()) && !reqs_validated {
+            try!(self.validate_required(matcher));
+        }
+        try!(self.validate_matched_args(matcher));
+        matcher.usage(usage::create_usage_with_title(self.0, &[]));
+
         Ok(())
     }
 

tests/app_settings.rs

@@ -110,6 +110,18 @@ fn arg_required_else_help() {
     assert_eq!(err.kind, ErrorKind::MissingArgumentOrSubcommand);
 }
 
+#[test]
+fn arg_required_else_help_over_reqs() {
+    let result = App::new("arg_required")
+        .setting(AppSettings::ArgRequiredElseHelp)
+        .arg(Arg::with_name("test")
+               .index(1).required(true))
+        .get_matches_from_safe(vec![""]);
+    assert!(result.is_err());
+    let err = result.err().unwrap();
+    assert_eq!(err.kind, ErrorKind::MissingArgumentOrSubcommand);
+}
+
 #[cfg(not(feature = "suggestions"))]
 #[test]
 fn infer_subcommands_fail_no_args() {

tests/positionals.rs

@@ -233,4 +233,14 @@ fn last_positional_no_double_dash() {
         .get_matches_from_safe(vec!["test", "tgt", "crp", "arg"]);
     assert!(r.is_err());
     assert_eq!(r.unwrap_err().kind, ErrorKind::UnknownArgument);
+}
+
+#[test]
+fn last_positional_second_to_last_mult() {
+    let r = App::new("test")
+        .arg_from_usage("<TARGET> 'some target'")
+        .arg_from_usage("[CORPUS]... 'some corpus'")
+        .arg(Arg::from_usage("[ARGS]... 'some file'").last(true))
+        .get_matches_from_safe(vec!["test", "tgt", "crp1", "crp2", "--", "arg"]);
+    assert!(r.is_ok(), "{:?}", r.unwrap_err().kind);
 }