Auto merge of #657 - kbknapp:issue-655, r=kbknapp

Issue 655

src/args/arg.rs

@@ -96,7 +96,7 @@ impl<'a, 'b> Default for Arg<'a, 'b> {
             validator: None,
             overrides: None,
             settings: ArgFlags::new(),
-            val_delim: Some(','),
+            val_delim: None,
             default_val: None,
             disp_ord: 999,
             r_unless: None,
@@ -1277,8 +1277,12 @@ impl<'a, 'b> Arg<'a, 'b> {
     /// [option]: ./struct.Arg.html#method.takes_value
     /// [`Arg::number_of_values(1)`]: ./struct.Arg.html#method.number_of_values
     /// [`Arg::multiple(true)`]: ./struct.Arg.html#method.multiple
-    pub fn multiple(self, multi: bool) -> Self {
+    pub fn multiple(mut self, multi: bool) -> Self {
         if multi {
+            if self.settings.is_set(ArgSettings::ValueDelimiterNotSet) &&
+                self.settings.is_set(ArgSettings::TakesValue) {
+                self = self.use_delimiter(true);
+            }
             self.set(ArgSettings::Multiple)
         } else {
             self.unset(ArgSettings::Multiple)
@@ -1649,6 +1653,13 @@ impl<'a, 'b> Arg<'a, 'b> {
     /// ```
     /// [`Arg::multiple(true)`]: ./struct.Arg.html#method.multiple
     pub fn number_of_values(mut self, qty: u64) -> Self {
+        if qty > 1 && self.settings.is_set(ArgSettings::ValueDelimiterNotSet) {
+            self.unsetb(ArgSettings::ValueDelimiterNotSet);
+            self.setb(ArgSettings::UseValueDelimiter);
+        } else {
+            self = self.use_delimiter(false);
+        }
+        self.setb(ArgSettings::TakesValue);
         self.num_vals = Some(qty);
         self
     }
@@ -1700,7 +1711,7 @@ impl<'a, 'b> Arg<'a, 'b> {
     ///
     /// **NOTE:** This does *not* implicitly set [`Arg::multiple(true)`]. This is because
     /// `-o val -o val` is multiple occurrences but a single value and `-o val1 val2` is a single
-    /// occurence with multple values. For positional arguments this **does** set
+    /// occurence with multiple values. For positional arguments this **does** set
     /// [`Arg::multiple(true)`] because there is no way to determine the difference between multiple
     /// occurences and multiple values.
     ///
@@ -1747,6 +1758,13 @@ impl<'a, 'b> Arg<'a, 'b> {
     /// ```
     /// [`Arg::multiple(true)`]: ./struct.Arg.html#method.multiple
     pub fn max_values(mut self, qty: u64) -> Self {
+        if qty > 1 && self.settings.is_set(ArgSettings::ValueDelimiterNotSet) {
+            self.unsetb(ArgSettings::ValueDelimiterNotSet);
+            self.setb(ArgSettings::UseValueDelimiter);
+        } else {
+            self = self.use_delimiter(false);
+        }
+        self.setb(ArgSettings::TakesValue);
         self.max_vals = Some(qty);
         self
     }
@@ -1814,8 +1832,12 @@ impl<'a, 'b> Arg<'a, 'b> {
     /// and `val3`) or as a single value (`val1,val2,val3`). Defaults to using `,` (comma) as the
     /// value delimiter for all arguments that accept values (options and positional arguments)
     ///
-    /// **NOTE:** The default is `true`. Setting the value to `true` will reset any previous use of
+    /// **NOTE:** The default is `false`. When set to `true` the default [`Arg::value_delimiter`]
-    /// [`Arg::value_delimiter`] back to the default of `,` (comma).
+    /// is the comma `,`.
+    ///
+    /// **NOTE:** When using methods like [`Arg::multiple`] or [`Arg::max_values`] (i.e. methods
+    /// that imply multiple values, this `use_delimiter` setting will automatically be set to
+    /// `true` and use the comma (`,`) by default.
     ///
     /// # Examples
     ///
@@ -1826,6 +1848,7 @@ impl<'a, 'b> Arg<'a, 'b> {
     /// let delims = App::new("delims")
     ///     .arg(Arg::with_name("option")
     ///         .long("option")
+    ///         .use_delimiter(true)
     ///         .takes_value(true))
     ///     .get_matches_from(vec![
     ///         "delims",
@@ -1836,7 +1859,8 @@ impl<'a, 'b> Arg<'a, 'b> {
     /// assert_eq!(delims.occurrences_of("option"), 1);
     /// assert_eq!(delims.values_of("option").unwrap().collect::<Vec<_>>(), ["val1", "val2", "val3"]);
     /// ```
-    /// The next example shows the difference when turning delimiters off.
+    /// The next example shows the difference when turning delimiters off. This is the default
+    /// behavior, unless one of the methods/settings which implies multiple values is set.
     ///
     /// ```rust
     /// # use clap::{App, Arg};
@@ -1857,11 +1881,16 @@ impl<'a, 'b> Arg<'a, 'b> {
     /// [`Arg::value_delimiter`]: ./struct.Arg.html#method.value_delimiter
     pub fn use_delimiter(mut self, d: bool) -> Self {
         if d {
+            if self.val_delim.is_none() {
                 self.val_delim = Some(',');
-            self.set(ArgSettings::UseValueDelimiter)
+            }
+            self.setb(ArgSettings::TakesValue);
+            self.setb(ArgSettings::UseValueDelimiter);
+            self.unset(ArgSettings::ValueDelimiterNotSet)
         } else {
             self.val_delim = None;
-            self.unset(ArgSettings::UseValueDelimiter)
+            self.unsetb(ArgSettings::UseValueDelimiter);
+            self.unset(ArgSettings::ValueDelimiterNotSet)
         }
     }
 
@@ -1939,6 +1968,7 @@ impl<'a, 'b> Arg<'a, 'b> {
     /// ```
     pub fn require_delimiter(mut self, d: bool) -> Self {
         if d {
+            self.unsetb(ArgSettings::ValueDelimiterNotSet);
             self.setb(ArgSettings::UseValueDelimiter);
             self.set(ArgSettings::RequireDelimiter)
         } else {
@@ -1972,8 +2002,9 @@ impl<'a, 'b> Arg<'a, 'b> {
     /// [`Arg::use_delimiter(true)`]: ./struct.Arg.html#method.use_delimiter
     /// [`Arg::takes_value(true)`]: ./struct.Arg.html#method.takes_value
     pub fn value_delimiter(mut self, d: &str) -> Self {
-        self = self.set(ArgSettings::TakesValue);
+        self.unsetb(ArgSettings::ValueDelimiterNotSet);
-        self = self.set(ArgSettings::UseValueDelimiter);
+        self.setb(ArgSettings::TakesValue);
+        self.setb(ArgSettings::UseValueDelimiter);
         self.val_delim = Some(d.chars()
             .nth(0)
             .expect("Failed to get value_delimiter from arg"));
@@ -2041,6 +2072,10 @@ impl<'a, 'b> Arg<'a, 'b> {
     /// [`Arg::multiple(true)`]: ./struct.Arg.html#method.multiple
     pub fn value_names(mut self, names: &[&'b str]) -> Self {
         self.setb(ArgSettings::TakesValue);
+        if self.settings.is_set(ArgSettings::ValueDelimiterNotSet) {
+            self.unsetb(ArgSettings::ValueDelimiterNotSet);
+            self.setb(ArgSettings::UseValueDelimiter);
+        }
         if let Some(ref mut vals) = self.val_names {
             let mut l = vals.len();
             for s in names {

src/args/settings.rs

@@ -4,16 +4,17 @@ use std::str::FromStr;
 
 bitflags! {
     flags Flags: u16 {
-        const REQUIRED       = 0b0000000001,
+        const REQUIRED       = 0b00000000001,
-        const MULTIPLE       = 0b0000000010,
+        const MULTIPLE       = 0b00000000010,
-        const EMPTY_VALS     = 0b0000000100,
+        const EMPTY_VALS     = 0b00000000100,
-        const GLOBAL         = 0b0000001000,
+        const GLOBAL         = 0b00000001000,
-        const HIDDEN         = 0b0000010000,
+        const HIDDEN         = 0b00000010000,
-        const TAKES_VAL      = 0b0000100000,
+        const TAKES_VAL      = 0b00000100000,
-        const USE_DELIM      = 0b0001000000,
+        const USE_DELIM      = 0b00001000000,
-        const NEXT_LINE_HELP = 0b0010000000,
+        const NEXT_LINE_HELP = 0b00010000000,
-        const R_UNLESS_ALL   = 0b0100000000,
+        const R_UNLESS_ALL   = 0b00100000000,
-        const REQ_DELIM      = 0b1000000000,
+        const REQ_DELIM      = 0b01000000000,
+        const DELIM_NOT_SET  = 0b10000000000,
     }
 }
 
@@ -36,13 +37,14 @@ impl ArgFlags {
         UseValueDelimiter => USE_DELIM,
         NextLineHelp => NEXT_LINE_HELP,
         RequiredUnlessAll => R_UNLESS_ALL,
-        RequireDelimiter => REQ_DELIM
+        RequireDelimiter => REQ_DELIM,
+        ValueDelimiterNotSet => DELIM_NOT_SET
     }
 }
 
 impl Default for ArgFlags {
     fn default() -> Self {
-        ArgFlags(EMPTY_VALS | USE_DELIM)
+        ArgFlags(EMPTY_VALS | DELIM_NOT_SET)
     }
 }
 
@@ -74,6 +76,8 @@ pub enum ArgSettings {
     RequireDelimiter,
     #[doc(hidden)]
     RequiredUnlessAll,
+    #[doc(hidden)]
+    ValueDelimiterNotSet,
 }
 
 impl FromStr for ArgSettings {
@@ -90,6 +94,7 @@ impl FromStr for ArgSettings {
             "nextlinehelp" => Ok(ArgSettings::NextLineHelp),
             "requiredunlessall" => Ok(ArgSettings::RequiredUnlessAll),
             "requiredelimiter" => Ok(ArgSettings::RequireDelimiter),
+            "valuedelimiternotset" => Ok(ArgSettings::ValueDelimiterNotSet),
             _ => Err("unknown ArgSetting, cannot convert from str".to_owned()),
         }
     }

src/usage_parser.rs

@@ -174,6 +174,13 @@ impl<'a> UsageParser<'a> {
                     if dot_counter == 3 {
                         debugln!("setting multiple");
                         arg.setb(ArgSettings::Multiple);
+                        if arg.settings.is_set(ArgSettings::TakesValue) {
+                            arg.setb(ArgSettings::UseValueDelimiter);
+                            arg.unsetb(ArgSettings::ValueDelimiterNotSet);
+                            if arg.val_delim.is_none() {
+                                arg.val_delim = Some(',');
+                            }
+                        }
                         self.prev = UsageToken::Multiple;
                         self.pos += 1;
                         break;

tests/app_settings.rs

@@ -223,7 +223,7 @@ fn delim_values_only_pos_follows() {
     let m = r.unwrap();
     assert!(m.is_present("arg"));
     assert!(!m.is_present("f"));
-    assert_eq!(m.values_of("arg").unwrap().collect::<Vec<_>>(), &["-f", "-g", "x"]);
+    assert_eq!(m.values_of("arg").unwrap().collect::<Vec<_>>(), &["-f", "-g,x"]);
 }
 
 #[test]
@@ -235,6 +235,31 @@ fn delim_values_trailingvararg() {
         )
         .get_matches_from(vec!["", "test", "--foo", "-Wl,-bar"]);
     assert!(m.is_present("opt"));
+    assert_eq!(m.values_of("opt").unwrap().collect::<Vec<_>>(), &["test", "--foo", "-Wl,-bar"]);
+}
+
+#[test]
+fn delim_values_only_pos_follows_with_delim() {
+    let r = App::new("onlypos")
+        .args(&[Arg::from_usage("-f [flag] 'some opt'"),
+                Arg::from_usage("[arg]... 'some arg'").use_delimiter(true)])
+        .get_matches_from_safe(vec!["", "--", "-f", "-g,x"]);
+    assert!(r.is_ok());
+    let m = r.unwrap();
+    assert!(m.is_present("arg"));
+    assert!(!m.is_present("f"));
+    assert_eq!(m.values_of("arg").unwrap().collect::<Vec<_>>(), &["-f", "-g", "x"]);
+}
+
+#[test]
+fn delim_values_trailingvararg_with_delim() {
+    let m = App::new("positional")
+        .setting(AppSettings::TrailingVarArg)
+        .arg(
+            Arg::from_usage("[opt]... 'some pos'").use_delimiter(true),
+        )
+        .get_matches_from(vec!["", "test", "--foo", "-Wl,-bar"]);
+    assert!(m.is_present("opt"));
     assert_eq!(m.values_of("opt").unwrap().collect::<Vec<_>>(), &["test", "--foo", "-Wl", "-bar"]);
 }
 

tests/multiple_occurrences.rs

@@ -5,7 +5,7 @@ use clap::{App, Arg};
 
 #[test]
 fn multiple_occurrences_of_flags_long() {
-    let m = App::new("multiple_occurrences")
+    let m = App::new("mo_flags_long")
                 .arg(Arg::from_usage("--multflag 'allowed multiple flag'")
                     .multiple(true))
                 .arg(Arg::from_usage("--flag 'disallowed multiple flag'"))
@@ -23,7 +23,7 @@ fn multiple_occurrences_of_flags_long() {
 
 #[test]
 fn multiple_occurrences_of_flags_short() {
-    let m = App::new("multiple_occurrences")
+    let m = App::new("mo_flags_short")
                 .arg(Arg::from_usage("-m --multflag 'allowed multiple flag'")
                     .multiple(true))
                 .arg(Arg::from_usage("-f --flag 'disallowed multiple flag'"))
@@ -41,7 +41,7 @@ fn multiple_occurrences_of_flags_short() {
 
 #[test]
 fn multiple_occurrences_of_flags_mixed() {
-    let m = App::new("multiple_occurrences")
+    let m = App::new("mo_flags_mixed")
                 .arg(Arg::from_usage("-m, --multflag1 'allowed multiple flag'")
                     .multiple(true))
                 .arg(Arg::from_usage("-n, --multflag2 'another allowed multiple flag'")
@@ -67,7 +67,7 @@ fn multiple_occurrences_of_flags_mixed() {
 #[test]
 fn multiple_occurrences_of_flags_large_quantity() {
     let args : Vec<&str> = vec![""].into_iter().chain(vec!["-m"; 1024].into_iter()).collect();
-    let m = App::new("multiple_occurrences")
+    let m = App::new("mo_flags_larg_qty")
                 .arg(Arg::from_usage("-m --multflag 'allowed multiple flag'")
                     .multiple(true))
                 .get_matches_from(args);

tests/multiple_values.rs

@@ -630,6 +630,7 @@ fn multiple_values_sep_positional() {
     let m = App::new("multiple_values")
         .arg(Arg::with_name("option")
             .help("multiple options")
+            .use_delimiter(true)
             .multiple(true))
         .get_matches_from_safe(vec![
             "",