Merge #1802

1802: Few ports from v2 r=CreepySkeleton a=pksunkara



Co-authored-by: Bence Kalmar <bkalmar1996@gmail.com>
Co-authored-by: Ivan Veselov <veselov@gmail.com>
Co-authored-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>

.travis.yml

@@ -38,7 +38,7 @@ jobs:
       cache: false
     - name: Release profile tests
       script:
-        - cargo test --release --features yaml unstable
+        - cargo test -v --release --features yaml unstable
     - name: Linting (fmt + clippy)
       before_script:
         - rustup component add clippy

src/build/arg/mod.rs

@@ -1986,7 +1986,7 @@ impl<'help> Arg<'help> {
     ///     ]);
     ///
     /// assert!(res.is_err());
-    /// assert_eq!(res.unwrap_err().kind, ErrorKind::TooManyValues);
+    /// assert_eq!(res.unwrap_err().kind, ErrorKind::UnknownArgument);
     /// ```
     /// [`Arg::multiple(true)`]: ./struct.Arg.html#method.multiple
     pub fn max_values(mut self, qty: u64) -> Self {

src/macros.rs

@@ -371,10 +371,11 @@ macro_rules! arg_enum {
         );
     };
     ($(#[$($m:meta),+])+ enum $e:ident { $($v:ident $(=$val:expr)*,)+ } ) => {
-        $crate::arg_enum!($(#[$($m:meta),+])+
-            enum $e:ident {
-                $($v:ident $(=$val:expr)*),+
-            }
+        $crate::arg_enum!(@impls
+            ($(#[$($m),+])+
+             enum $e {
+                 $($v$(=$val)*),+
+             }) -> ($e, $($v),+)
         );
     };
     ($(#[$($m:meta),+])+ enum $e:ident { $($v:ident $(=$val:expr)*),+ } ) => {
@@ -386,9 +387,11 @@ macro_rules! arg_enum {
         );
     };
     (pub enum $e:ident { $($v:ident $(=$val:expr)*,)+ } ) => {
-        $crate::arg_enum!(pub enum $e:ident {
-            $($v:ident $(=$val:expr)*),+
-        });
+        $crate::arg_enum!(@impls
+            (pub enum $e {
+                $($v$(=$val)*),+
+            }) -> ($e, $($v),+)
+        );
     };
     (pub enum $e:ident { $($v:ident $(=$val:expr)*),+ } ) => {
         $crate::arg_enum!(@impls
@@ -398,9 +401,11 @@ macro_rules! arg_enum {
         );
     };
     (enum $e:ident { $($v:ident $(=$val:expr)*,)+ } ) => {
-        $crate::arg_enum!(enum $e:ident {
-            $($v:ident $(=$val:expr)*),+
-        });
+        $crate::arg_enum!(@impls
+            (enum $e {
+                $($v$(=$val)*),+
+            }) -> ($e, $($v),+)
+        );
     };
     (enum $e:ident { $($v:ident $(=$val:expr)*),+ } ) => {
         $crate::arg_enum!(@impls

src/parse/arg_matcher.rs

@@ -173,7 +173,7 @@ impl ArgMatcher {
                 };
             } else if let Some(num) = o.max_vals {
                 debugln!("ArgMatcher::needs_more_vals: max_vals...{}", num);
-                return (ma.vals.len() as u64) <= num;
+                return (ma.vals.len() as u64) < num;
             } else if o.min_vals.is_some() {
                 debugln!("ArgMatcher::needs_more_vals: min_vals...true");
                 return true;

tests/macros.rs

@@ -286,24 +286,115 @@ fn group_macro_set_not_required() {
 
 #[test]
 fn arg_enum() {
-    arg_enum! {
+    // Helper macros to avoid repetition
+    macro_rules! test_greek {
+        ($arg_enum:item, $tests:block) => {{
+            $arg_enum
+            // FromStr implementation
+            assert!("Charlie".parse::<Greek>().is_err());
+            // Display implementation
+            assert_eq!(format!("{}", Greek::Alpha), "Alpha");
+            assert_eq!(format!("{}", Greek::Bravo), "Bravo");
+            // fn variants()
+            assert_eq!(Greek::variants(), ["Alpha", "Bravo"]);
+            // rest of tests
+            $tests
+        }};
+    }
+    macro_rules! test_greek_no_meta {
+        {$arg_enum:item} => {
+            test_greek!($arg_enum, {
+                // FromStr implementation
+                assert!("Alpha".parse::<Greek>().is_ok());
+                assert!("Bravo".parse::<Greek>().is_ok());
+            })
+        };
+    }
+    macro_rules! test_greek_meta {
+        {$arg_enum:item} => {
+            test_greek!($arg_enum, {
+                // FromStr implementation
+                assert_eq!("Alpha".parse::<Greek>(), Ok(Greek::Alpha));
+                assert_eq!("Bravo".parse::<Greek>(), Ok(Greek::Bravo));
+            })
+        };
+    }
+
+    // Tests for each pattern
+    // meta  NO, pub  NO, trailing comma  NO
+    test_greek_no_meta! {
+        arg_enum!{
+            enum Greek {
+                Alpha,
+                Bravo
+            }
+        }
+    };
+    // meta  NO, pub  NO, trailing comma YES
+    test_greek_no_meta! {
+        arg_enum!{
+            enum Greek {
+                Alpha,
+                Bravo,
+            }
+        }
+    };
+    // meta  NO, pub YES, trailing comma  NO
+    test_greek_no_meta! {
+        arg_enum!{
+            pub enum Greek {
+                Alpha,
+                Bravo
+            }
+        }
+    };
+    // meta  NO, pub YES, trailing comma YES
+    test_greek_no_meta! {
+        arg_enum!{
+            pub enum Greek {
+                Alpha,
+                Bravo,
+            }
+        }
+    };
+    // meta YES, pub  NO, trailing comma  NO
+    test_greek_meta! {
+        arg_enum!{
+            #[derive(Debug, PartialEq, Copy, Clone)]
+            enum Greek {
+                Alpha,
+                Bravo
+            }
+        }
+    };
+    // meta YES, pub  NO, trailing comma YES
+    test_greek_meta! {
+        arg_enum!{
+            #[derive(Debug, PartialEq, Copy, Clone)]
+            enum Greek {
+                Alpha,
+                Bravo,
+            }
+        }
+    };
+    // meta YES, pub YES, trailing comma  NO
+    test_greek_meta! {
+        arg_enum!{
             #[derive(Debug, PartialEq, Copy, Clone)]
             pub enum Greek {
                 Alpha,
                 Bravo
             }
         }
-    assert_eq!("Alpha".parse::<Greek>(), Ok(Greek::Alpha));
-}
-
-#[test]
-fn arg_enum_trailing_comma() {
-    arg_enum! {
+    };
+    // meta YES, pub YES, trailing comma YES
+    test_greek_meta! {
+        arg_enum!{
             #[derive(Debug, PartialEq, Copy, Clone)]
             pub enum Greek {
                 Alpha,
                 Bravo,
             }
         }
-    assert_eq!("Alpha".parse::<Greek>(), Ok(Greek::Alpha));
+    };
 }

tests/multiple_values.rs

@@ -1212,3 +1212,51 @@ fn multiple_vals_with_hyphen() {
     assert_eq!(&cmds, &["find", "-type", "f", "-name", "special"]);
     assert_eq!(m.value_of("location"), Some("/home/clap"));
 }
+
+#[test]
+fn issue_1480_max_values_consumes_extra_arg_1() {
+    let res = App::new("prog")
+        .arg(
+            Arg::with_name("field")
+                .takes_value(true)
+                .multiple(false)
+                .max_values(1)
+                .long("field"),
+        )
+        .arg(Arg::with_name("positional").required(true).index(1))
+        .try_get_matches_from(vec!["prog", "--field", "1", "file"]);
+
+    assert!(res.is_ok());
+}
+
+#[test]
+fn issue_1480_max_values_consumes_extra_arg_2() {
+    let res = App::new("prog")
+        .arg(
+            Arg::with_name("field")
+                .takes_value(true)
+                .multiple(false)
+                .max_values(1)
+                .long("field"),
+        )
+        .try_get_matches_from(vec!["prog", "--field", "1", "2"]);
+
+    assert!(res.is_err());
+    assert_eq!(res.unwrap_err().kind, ErrorKind::UnknownArgument);
+}
+
+#[test]
+fn issue_1480_max_values_consumes_extra_arg_3() {
+    let res = App::new("prog")
+        .arg(
+            Arg::with_name("field")
+                .takes_value(true)
+                .multiple(false)
+                .max_values(1)
+                .long("field"),
+        )
+        .try_get_matches_from(vec!["prog", "--field", "1", "2", "3"]);
+
+    assert!(res.is_err());
+    assert_eq!(res.unwrap_err().kind, ErrorKind::UnknownArgument);
+}