Repository: kbknapp/clap-rs
Pull request #1203 (2018-03-06 15:43:53)

Skipable Positional Args with AppSettings::AllowMissingPositional

cc @matklad

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/kbknapp/clap-rs/1203)
<!-- Reviewable:end -->



Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c950611c..71b38f5d 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -1,3 +1,12 @@
+<a name="v2.30.1"></a>
+### v2.30.1 (2018-03-06)
+
+
+#### Improvements
+
+* **AllowMissingPositional:**  improves the ability of AllowMissingPositional to allow 'skipping' to the last positional arg with '--' ([df20e6e2](https://github.com/kbknapp/clap-rs/commit/df20e6e24b4e782be0b423b484b9798e3e2efe2f))
+
+
 <a name="v2.31.0"></a>
 ## v2.31.0 (2018-03-04)
 
diff --git a/Cargo.toml b/Cargo.toml
index 1d3c5f88..c5d767fd 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -1,7 +1,7 @@
 [package]
 
 name = "clap"
-version = "2.31.0"
+version = "2.31.1"
 authors = ["Kevin K. <kbknapp@gmail.com>"]
 exclude = ["examples/*", "clap-test/*", "tests/*", "benches/*", "*.png", "clap-perf/*", "*.dot"]
 repository = "https://github.com/kbknapp/clap-rs"
diff --git a/README.md b/README.md
index 0c091857..4011b316 100644
--- a/README.md
+++ b/README.md
@@ -42,6 +42,10 @@ Created by [gh-md-toc](https://github.com/ekalinin/github-markdown-toc)
 
 ## What's New
 
+Here's whats new in 2.31.1:
+
+* **AllowMissingPositional:**  improves the ability of `AppSetting::AllowMissingPositional` to allow "skipping" to the last positional arg with the `--` operator
+
 Here's whats new in 2.31.0:
 
 * **Arg Indices:**  adds the ability to query argument value indices
@@ -540,7 +544,7 @@ Right now Cargo's version resolution is pretty naive, it's just a brute-force se
 
 # In one Cargo.toml
 [dependencies]
-clap = "~2.31.0"
+clap = "~2.31.1"
 
 # In another Cargo.toml
 [dependencies]
diff --git a/src/app/parser.rs b/src/app/parser.rs
index d4b7d961..b517b4f7 100644
--- a/src/app/parser.rs
+++ b/src/app/parser.rs
@@ -444,9 +444,7 @@ where
         }
     }
 
-    pub fn required(&self) -> Iter<&str> {
-        self.required.iter()
-    }
+    pub fn required(&self) -> Iter<&str> { self.required.iter() }
 
     #[cfg_attr(feature = "lints", allow(needless_borrow))]
     #[inline]
@@ -455,24 +453,16 @@ where
     }
 
     #[inline]
-    pub fn has_opts(&self) -> bool {
-        !self.opts.is_empty()
-    }
+    pub fn has_opts(&self) -> bool { !self.opts.is_empty() }
 
     #[inline]
-    pub fn has_flags(&self) -> bool {
-        !self.flags.is_empty()
-    }
+    pub fn has_flags(&self) -> bool { !self.flags.is_empty() }
 
     #[inline]
-    pub fn has_positionals(&self) -> bool {
-        !self.positionals.is_empty()
-    }
+    pub fn has_positionals(&self) -> bool { !self.positionals.is_empty() }
 
     #[inline]
-    pub fn has_subcommands(&self) -> bool {
-        !self.subcommands.is_empty()
-    }
+    pub fn has_subcommands(&self) -> bool { !self.subcommands.is_empty() }
 
     #[inline]
     pub fn has_visible_opts(&self) -> bool {
@@ -510,19 +500,13 @@ where
     }
 
     #[inline]
-    pub fn is_set(&self, s: AS) -> bool {
-        self.settings.is_set(s)
-    }
+    pub fn is_set(&self, s: AS) -> bool { self.settings.is_set(s) }
 
     #[inline]
-    pub fn set(&mut self, s: AS) {
-        self.settings.set(s)
-    }
+    pub fn set(&mut self, s: AS) { self.settings.set(s) }
 
     #[inline]
-    pub fn unset(&mut self, s: AS) {
-        self.settings.unset(s)
-    }
+    pub fn unset(&mut self, s: AS) { self.settings.unset(s) }
 
     #[cfg_attr(feature = "lints", allow(block_in_if_condition_stmt))]
     pub fn verify_positionals(&mut self) -> bool {
@@ -1010,7 +994,8 @@ where
             let low_index_mults = self.is_set(AS::LowIndexMultiplePositional)
                 && pos_counter == (self.positionals.len() - 1);
             let missing_pos = self.is_set(AS::AllowMissingPositional)
-                && pos_counter == (self.positionals.len() - 1);
+                && (pos_counter == (self.positionals.len() - 1) 
+                    && !self.is_set(AS::TrailingValues));
             debugln!(
                 "Parser::get_matches_with: Positional counter...{}",
                 pos_counter
@@ -1043,7 +1028,9 @@ where
                     debugln!("Parser::get_matches_with: Bumping the positional counter...");
                     pos_counter += 1;
                 }
-            } else if self.is_set(AS::ContainsLast) && self.is_set(AS::TrailingValues) {
+            } else if (self.is_set(AS::AllowMissingPositional) && self.is_set(AS::TrailingValues))
+                || (self.is_set(AS::ContainsLast) && self.is_set(AS::TrailingValues))
+            {
                 // Came to -- and one postional has .last(true) set, so we go immediately
                 // to the last (highest index) positional
                 debugln!("Parser::get_matches_with: .last(true) and --, setting last pos");
@@ -2047,21 +2034,13 @@ where
         Ok(())
     }
 
-    pub fn flags(&self) -> Iter<FlagBuilder<'a, 'b>> {
-        self.flags.iter()
-    }
+    pub fn flags(&self) -> Iter<FlagBuilder<'a, 'b>> { self.flags.iter() }
 
-    pub fn opts(&self) -> Iter<OptBuilder<'a, 'b>> {
-        self.opts.iter()
-    }
+    pub fn opts(&self) -> Iter<OptBuilder<'a, 'b>> { self.opts.iter() }
 
-    pub fn positionals(&self) -> map::Values<PosBuilder<'a, 'b>> {
-        self.positionals.values()
-    }
+    pub fn positionals(&self) -> map::Values<PosBuilder<'a, 'b>> { self.positionals.values() }
 
-    pub fn subcommands(&self) -> Iter<App> {
-        self.subcommands.iter()
-    }
+    pub fn subcommands(&self) -> Iter<App> { self.subcommands.iter() }
 
     // Should we color the output? None=determined by output location, true=yes, false=no
     #[doc(hidden)]
@@ -2145,12 +2124,8 @@ where
     }
 
     #[inline]
-    fn contains_long(&self, l: &str) -> bool {
-        longs!(self).any(|al| al == &l)
-    }
+    fn contains_long(&self, l: &str) -> bool { longs!(self).any(|al| al == &l) }
 
     #[inline]
-    fn contains_short(&self, s: char) -> bool {
-        shorts!(self).any(|arg_s| arg_s == &s)
-    }
+    fn contains_short(&self, s: char) -> bool { shorts!(self).any(|arg_s| arg_s == &s) }
 }
diff --git a/src/app/settings.rs b/src/app/settings.rs
index aef2a587..e676b7b8 100644
--- a/src/app/settings.rs
+++ b/src/app/settings.rs
@@ -221,7 +221,9 @@ pub enum AppSettings {
     /// [`AllowLeadingHyphen`]: ./enum.AppSettings.html#variant.AllowLeadingHyphen
     AllowNegativeNumbers,
 
-    /// Allows one to implement a CLI where the second to last positional argument is optional, but
+    /// Allows one to implement two styles of CLIs where positionals can be used out of order.
+    ///
+    /// The first example is a CLI where the second to last positional argument is optional, but
     /// the final positional argument is required. Such as `$ prog [optional] <required>` where one
     /// of the two following usages is allowed:
     ///
@@ -230,11 +232,46 @@ pub enum AppSettings {
     ///
     /// This would otherwise not be allowed. This is useful when `[optional]` has a default value.
     ///
-    /// **Note:** In addition to using this setting, the second positional argument *must* be
-    /// [required]
+    /// **Note:** when using this style of "missing positionals" the final positional *must* be
+    /// [required] if `--` will not be used to skip to the final positional argument.
+    ///
+    /// **Note:** This style also only allows a single positional argument to be "skipped" without
+    /// the use of `--`. To skip more than one, see the second example.
+    ///
+    /// The second example is when one wants to skip multiple optional positional arguments, and use
+    /// of the `--` operator is OK (but not required if all arguments will be specified anyways).
+    ///
+    /// For example, imagine a CLI which has three positional arguments `[foo] [bar] [baz]...` where
+    /// `baz` accepts multiple values (similar to man `ARGS...` style training arguments).
+    ///
+    /// With this setting the following invocations are posisble:
+    ///
+    /// * `$ prog foo bar baz1 baz2 baz3`
+    /// * `$ prog foo -- baz1 baz2 baz3`
+    /// * `$ prog -- baz1 baz2 baz3`
     ///
     /// # Examples
     ///
+    /// Style number one from above:
+    ///
+    /// ```rust
+    /// # use clap::{App, Arg, AppSettings};
+    /// // Assume there is an external subcommand named "subcmd"
+    /// let m = App::new("myprog")
+    ///     .setting(AppSettings::AllowMissingPositional)
+    ///     .arg(Arg::with_name("arg1"))
+    ///     .arg(Arg::with_name("arg2")
+    ///         .required(true))
+    ///     .get_matches_from(vec![
+    ///         "prog", "other"
+    ///     ]);
+    ///
+    /// assert_eq!(m.value_of("arg1"), None);
+    /// assert_eq!(m.value_of("arg2"), Some("other"));
+    /// ```
+    ///
+    /// Now the same example, but using a default value for the first optional positional argument
+    ///
     /// ```rust
     /// # use clap::{App, Arg, AppSettings};
     /// // Assume there is an external subcommand named "subcmd"
@@ -245,12 +282,49 @@ pub enum AppSettings {
     ///     .arg(Arg::with_name("arg2")
     ///         .required(true))
     ///     .get_matches_from(vec![
-    ///         "myprog", "other"
+    ///         "prog", "other"
     ///     ]);
     ///
     /// assert_eq!(m.value_of("arg1"), Some("something"));
     /// assert_eq!(m.value_of("arg2"), Some("other"));
     /// ```
+    /// Style number two from above:
+    ///
+    /// ```rust
+    /// # use clap::{App, Arg, AppSettings};
+    /// // Assume there is an external subcommand named "subcmd"
+    /// let m = App::new("myprog")
+    ///     .setting(AppSettings::AllowMissingPositional)
+    ///     .arg(Arg::with_name("foo"))
+    ///     .arg(Arg::with_name("bar"))
+    ///     .arg(Arg::with_name("baz").multiple(true))
+    ///     .get_matches_from(vec![
+    ///         "prog", "foo", "bar", "baz1", "baz2", "baz3"
+    ///     ]);
+    ///
+    /// assert_eq!(m.value_of("foo"), Some("foo"));
+    /// assert_eq!(m.value_of("bar"), Some("bar"));
+    /// assert_eq!(m.values_of("baz").unwrap().collect::<Vec<_>>(), &["baz1", "baz2", "baz3"]);
+    /// ```
+    ///
+    /// Now nofice if we don't specifiy `foo` or `baz` but use the `--` operator.
+    ///
+    /// ```rust
+    /// # use clap::{App, Arg, AppSettings};
+    /// // Assume there is an external subcommand named "subcmd"
+    /// let m = App::new("myprog")
+    ///     .setting(AppSettings::AllowMissingPositional)
+    ///     .arg(Arg::with_name("foo"))
+    ///     .arg(Arg::with_name("bar"))
+    ///     .arg(Arg::with_name("baz").multiple(true))
+    ///     .get_matches_from(vec![
+    ///         "prog", "--", "baz1", "baz2", "baz3"
+    ///     ]);
+    ///
+    /// assert_eq!(m.value_of("foo"), None);
+    /// assert_eq!(m.value_of("bar"), None);
+    /// assert_eq!(m.values_of("baz").unwrap().collect::<Vec<_>>(), &["baz1", "baz2", "baz3"]);
+    /// ```
     /// [required]: ./struct.Arg.html#method.required
     AllowMissingPositional,
 
diff --git a/src/lib.rs b/src/lib.rs
index 1a76d6f6..09ec4191 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -513,7 +513,7 @@
 //! this repository for more information.
 
 #![crate_type = "lib"]
-#![doc(html_root_url = "https://docs.rs/clap/2.31.0")]
+#![doc(html_root_url = "https://docs.rs/clap/2.31.1")]
 #![deny(missing_docs, missing_debug_implementations, missing_copy_implementations, trivial_casts,
         unused_import_braces, unused_allocation)]
 // Lints we'd like to deny but are currently failing for upstream crates


Test patch:
diff --git a/tests/app_settings.rs b/tests/app_settings.rs
index 2168c500..80a4cfeb 100644
--- a/tests/app_settings.rs
+++ b/tests/app_settings.rs
@@ -629,6 +629,91 @@ fn allow_missing_positional() {
     assert_eq!(m.value_of("dest"), Some("file"));
 }
 
+#[test]
+fn allow_missing_positional_no_default() {
+    let m = App::new("test")
+        .setting(AppSettings::AllowMissingPositional)
+        .arg(Arg::from_usage("[src] 'some file'"))
+        .arg_from_usage("<dest> 'some file'")
+        .get_matches_from_safe(vec!["test", "file"]);
+    assert!(m.is_ok(), "{:?}", m.unwrap_err().kind);
+    let m = m.unwrap();
+    assert_eq!(m.value_of("src"), None);
+    assert_eq!(m.value_of("dest"), Some("file"));
+}
+
+#[test]
+fn missing_positional_no_hyphen() {
+    let r = App::new("bench")
+        .setting(AppSettings::AllowMissingPositional)
+        .arg(Arg::from_usage("[BENCH] 'some bench'"))
+        .arg(Arg::from_usage("[ARGS]... 'some args'"))
+        .get_matches_from_safe(vec!["bench", "foo", "arg1", "arg2", "arg3"]);
+    assert!(r.is_ok(), "{:?}", r.unwrap_err().kind);
+
+    let m = r.unwrap();
+
+    let expected_bench = Some("foo");
+    let expected_args = vec!["arg1", "arg2", "arg3"];
+
+    assert_eq!(m.value_of("BENCH"), expected_bench);
+    assert_eq!(m.values_of("ARGS").unwrap().collect::<Vec<_>>(), &*expected_args);
+}
+
+#[test]
+fn missing_positional_hyphen() {
+    let r = App::new("bench")
+        .setting(AppSettings::AllowMissingPositional)
+        .arg(Arg::from_usage("[BENCH] 'some bench'"))
+        .arg(Arg::from_usage("[ARGS]... 'some args'"))
+        .get_matches_from_safe(vec!["bench", "--", "arg1", "arg2", "arg3"]);
+    assert!(r.is_ok(), "{:?}", r.unwrap_err().kind);
+
+    let m = r.unwrap();
+
+    let expected_bench = None;
+    let expected_args = vec!["arg1", "arg2", "arg3"];
+
+    assert_eq!(m.value_of("BENCH"), expected_bench);
+    assert_eq!(m.values_of("ARGS").unwrap().collect::<Vec<_>>(), &*expected_args);
+}
+
+#[test]
+fn missing_positional_hyphen_far_back() {
+    let r = App::new("bench")
+        .setting(AppSettings::AllowMissingPositional)
+        .arg(Arg::from_usage("[BENCH1] 'some bench'"))
+        .arg(Arg::from_usage("[BENCH2] 'some bench'"))
+        .arg(Arg::from_usage("[BENCH3] 'some bench'"))
+        .arg(Arg::from_usage("[ARGS]... 'some args'"))
+        .get_matches_from_safe(vec!["bench", "foo", "--", "arg1", "arg2", "arg3"]);
+    assert!(r.is_ok(), "{:?}", r.unwrap_err().kind);
+
+    let m = r.unwrap();
+
+    let expected_bench1 = Some("foo");
+    let expected_bench2 = None;
+    let expected_bench3 = None;
+    let expected_args = vec!["arg1", "arg2", "arg3"];
+
+    assert_eq!(m.value_of("BENCH1"), expected_bench1);
+    assert_eq!(m.value_of("BENCH2"), expected_bench2);
+    assert_eq!(m.value_of("BENCH3"), expected_bench3);
+    assert_eq!(m.values_of("ARGS").unwrap().collect::<Vec<_>>(), &*expected_args);
+}
+
+#[test]
+fn missing_positional_hyphen_req_error() {
+    let r = App::new("bench")
+        .setting(AppSettings::AllowMissingPositional)
+        .arg(Arg::from_usage("[BENCH1] 'some bench'"))
+        .arg(Arg::from_usage("<BENCH2> 'some bench'"))
+        .arg(Arg::from_usage("[ARGS]... 'some args'"))
+        .get_matches_from_safe(vec!["bench", "foo", "--", "arg1", "arg2", "arg3"]);
+    assert!(r.is_err());
+    assert_eq!(r.unwrap_err().kind, ErrorKind::MissingRequiredArgument);
+}
+
 #[test]
 fn issue_1066_allow_leading_hyphen_and_unknown_args() {
     let res = App::new("prog")
diff --git a/tests/positionals.rs b/tests/positionals.rs
index 5e37668b..bf0f79fa 100644
--- a/tests/positionals.rs
+++ b/tests/positionals.rs
@@ -271,4 +271,4 @@ fn last_positional_second_to_last_mult() {
         .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);
-}
\ No newline at end of file
+}