From dd2a75640ca68a91b973faad15f04df891356cef Mon Sep 17 00:00:00 2001 From: Kevin K Date: Mon, 4 May 2015 20:22:57 -0400 Subject: [PATCH] fix(MultipleValues): properly distinguishes between multiple values and multiple occurrences When using number_of_values() or value_names() you no longer have to set .multiple(true) unless you want the argument to be allowed multiple times (i.e. *not* the value, but the argument itself). This means without multiple(true) set (and assuming 2 values) -o val1 val2 is only allowed one time, but with multiple(true) set, -o val1 val2 -o val3 val4 is allowed. Closes #99 --- src/app.rs | 81 ++++++++++++++++++++++++++++++------------------- src/args/arg.rs | 35 ++++++++++++++++++--- 2 files changed, 80 insertions(+), 36 deletions(-) diff --git a/src/app.rs b/src/app.rs index e0ee2c7c..86f29571 100644 --- a/src/app.rs +++ b/src/app.rs @@ -315,7 +315,11 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ max_vals: a.max_vals, help: a.help, }; - if pb.num_vals.unwrap_or(0) > 1 && !pb.multiple { + if pb.min_vals.is_some() && !pb.multiple { + panic!("Argument \"{}\" does not allow multiple values, yet it is expecting {} \ + values", pb.name, pb.num_vals.unwrap()); + } + if pb.max_vals.is_some() && !pb.multiple { panic!("Argument \"{}\" does not allow multiple values, yet it is expecting {} \ values", pb.name, pb.num_vals.unwrap()); } @@ -332,7 +336,7 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ let mut rhs = HashSet::new(); // without derefing n = &&str for n in r { - rhs.insert(*n); + rhs.insert(*n); if pb.required { self.required.insert(*n); } @@ -371,7 +375,11 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ if let Some(ref vec) = ob.val_names { ob.num_vals = Some(vec.len() as u8); } - if ob.num_vals.unwrap_or(0) > 1 && !ob.multiple { + if ob.min_vals.is_some() && !ob.multiple { + panic!("Argument \"{}\" does not allow multiple values, yet it is expecting {} \ + values", ob.name, ob.num_vals.unwrap()); + } + if ob.max_vals.is_some() && !ob.multiple { panic!("Argument \"{}\" does not allow multiple values, yet it is expecting {} \ values", ob.name, ob.num_vals.unwrap()); } @@ -387,8 +395,8 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ if let Some(ref r) = a.requires { let mut rhs = HashSet::new(); // without derefing n = &&str - for n in r { - rhs.insert(*n); + for n in r { + rhs.insert(*n); if ob.required { self.required.insert(*n); } @@ -683,8 +691,8 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ } if g_vec.is_empty() { - return args.iter().map(|s| s.to_owned()).collect::>() - } + return args.iter().map(|s| s.to_owned()).collect::>() + } return g_vec.iter().map(|g| self.get_group_members(g)).fold(vec![], |acc, v| acc + &v) } @@ -707,8 +715,8 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ } if g_vec.is_empty() { - return args.iter().map(|s| *s).collect::>() - } + return args.iter().map(|s| *s).collect::>() + } return g_vec.iter() .map(|g| self.get_group_members_names(g)) .fold(vec![], |acc, v| acc + &v) @@ -841,7 +849,7 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ let mut hs = self.required.iter().map(|n| *n).collect::>(); tmp_vec.iter().map(|n| hs.insert(*n)).collect::>(); let reqs = self.get_required_from(hs); - + let r_string = reqs.iter().fold(String::new(), |acc, s| acc + &format!(" {}", s)[..]); usage.push_str(&format!("{}{}", @@ -1203,11 +1211,11 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ if let Some(num) = opt.num_vals { if let Some(ref ma) = matches.args.get(opt.name) { if let Some(ref vals) = ma.values { - if num == vals.len() as u8 { + if num == vals.len() as u8 && !opt.multiple { self.report_error(format!("The argument \"{}\" was found, \ - but '{}' only expects {} values", - arg, - opt, + but '{}' only expects {} values", + arg, + opt, vals.len()), true, true, @@ -1401,10 +1409,10 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ match needs_val_of { Some(ref a) => { if let Some(o) = self.opts.get(a) { - if o.multiple && self.required.is_empty() { + if o.multiple && self.required.is_empty() { let should_err = match matches.values_of(o.name) { Some(ref v) => if v.len() == 0 { true } else { false }, - None => true, + None => true, }; if should_err { self.report_error( @@ -1423,14 +1431,14 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ } else { self.report_error(format!("The following required arguments were not \ - supplied:\n{}", + supplied:\n{}", self.get_required_from(self.required.iter() .map(|s| *s) .collect::>()) .iter() .fold(String::new(), |acc, s| acc + &format!("\t'{}'\n",s)[..])), - true, - true, + true, + true, Some(matches.args.keys().map(|k| *k).collect::>())); } } else { @@ -1452,14 +1460,14 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ if !self.required.is_empty() { if self.validate_required(&matches) { self.report_error(format!("The following required arguments were not \ - supplied:\n{}", + supplied:\n{}", self.get_required_from(self.required.iter() .map(|s| *s) .collect::>()) .iter() .fold(String::new(), |acc, s| acc + &format!("\t'{}'\n",s)[..])), - true, - true, + true, + true, Some(matches.args.keys().map(|k| *k).collect::>())); } } @@ -1741,7 +1749,7 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ } // Shouldn't reach here - self.report_error(format!("Argument --{} isn't valid", arg), + self.report_error(format!("The argument --{} isn't valid", arg), true, true, Some(matches.args.keys().map(|k| *k).collect::>())); @@ -1757,7 +1765,7 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ for c in arg.chars() { self.check_for_help_and_version(c); if !self.parse_single_short_flag(matches, c) { - self.report_error(format!("Argument -{} isn't valid",arg), + self.report_error(format!("The argument -{} isn't valid",arg), true, true, Some(matches.args.keys().map(|k| *k).collect::>())); @@ -1795,8 +1803,8 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ if matches.args.contains_key(v.name) { if !v.multiple { - self.report_error(format!("Argument -{} was supplied more than once, but does \ - not support multiple values", arg), + self.report_error(format!("The argument -{} was supplied more than once, but \ + does not support multiple values", arg), true, true, Some(matches.args.keys().map(|k| *k).collect::>())); @@ -1834,7 +1842,7 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ } // Didn't match a flag or option, must be invalid - self.report_error( format!("Argument -{} isn't valid",arg_c), + self.report_error( format!("The argument -{} isn't valid",arg_c), true, true, Some(matches.args.keys().map(|k| *k).collect::>())); @@ -1863,7 +1871,7 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ // Make sure this isn't one being added multiple times if it doesn't suppor it if matches.args.contains_key(v.name) && !v.multiple { - self.report_error(format!("Argument -{} was supplied more than once, but does \ + self.report_error(format!("The argument -{} was supplied more than once, but does \ not support multiple values", arg), true, true, @@ -1961,13 +1969,24 @@ impl<'a, 'v, 'ab, 'u, 'h, 'ar> App<'a, 'v, 'ab, 'u, 'h, 'ar>{ if let Some(ref vals) = ma.values { if let Some(f) = self.opts.get(name) { if let Some(num) = f.num_vals { - if num != vals.len() as u8 { + let should_err = if f.multiple { + ((vals.len() as u8) % num) != 0 + } else { + num != (vals.len() as u8) + }; + if should_err { self.report_error(format!("The argument '{}' requires {} values, \ but {} w{} provided", f, num, - vals.len(), - if vals.len() == 1 {"as"}else{"ere"}), + if f.multiple { + vals.len() % num as usize + } else { + vals.len() + }, + if vals.len() == 1 || + ( f.multiple && + ( vals.len() % num as usize) == 1) {"as"}else{"ere"}), true, true, Some(matches.args.keys().map(|k| *k).collect::>())); diff --git a/src/args/arg.rs b/src/args/arg.rs index 5ceb7742..3063850c 100644 --- a/src/args/arg.rs +++ b/src/args/arg.rs @@ -654,9 +654,11 @@ impl<'n, 'l, 'h, 'g, 'p, 'r> Arg<'n, 'l, 'h, 'g, 'p, 'r> { /// `.number_of_values(3)`, and this argument wouldn't be satisfied unless the user provided /// 3 and only 3 values. /// - /// **NOTE:** The argument *must* have `.multiple(true)` or `...` to use this setting. Which - /// implies that `qty` must be > 1 (i.e. setting `.number_of_values(1)` would be unnecessary) + /// **NOTE:** `qty` must be > 1 /// + /// **NOTE:** Does *not* require `.multiple(true)` to be set. Setting `.multiple(true)` would + /// allow `-f -f ` where as *not* setting + /// `.multiple(true)` would only allow one occurrence of this argument. /// /// # Example /// @@ -668,6 +670,11 @@ impl<'n, 'l, 'h, 'g, 'p, 'r> Arg<'n, 'l, 'h, 'g, 'p, 'r> { /// .number_of_values(3) /// # ).get_matches(); pub fn number_of_values(mut self, qty: u8) -> Arg<'n, 'l, 'h, 'g, 'p, 'r> { + if qty < 2 { + panic!("Arguments with number_of_values(qty) qty must be > 1. Prefer \ + takes_value(true) for arguments with onyl one value, or flags for arguments \ + with 0 values."); + } self.num_vals = Some(qty); self } @@ -677,8 +684,9 @@ impl<'n, 'l, 'h, 'g, 'p, 'r> Arg<'n, 'l, 'h, 'g, 'p, 'r> { /// `.max_values(3)`, and this argument would be satisfied if the user provided, 1, 2, or 3 /// values. /// - /// **NOTE:** The argument *must* have `.multiple(true)` or `...` to use this setting. Which - /// implies that `qty` must be > 1 (i.e. setting `.max_values(1)` would be unnecessary) + /// **NOTE:** `qty` must be > 1 + /// + /// **NOTE:** This implicity sets `.multiple(true)` /// /// # Example /// @@ -690,7 +698,13 @@ impl<'n, 'l, 'h, 'g, 'p, 'r> Arg<'n, 'l, 'h, 'g, 'p, 'r> { /// .max_values(3) /// # ).get_matches(); pub fn max_values(mut self, qty: u8) -> Arg<'n, 'l, 'h, 'g, 'p, 'r> { + if qty < 2 { + panic!("Arguments with max_values(qty) qty must be > 1. Prefer \ + takes_value(true) for arguments with onyl one value, or flags for arguments \ + with 0 values."); + } self.max_vals = Some(qty); + self.multiple = true; self } @@ -699,7 +713,9 @@ impl<'n, 'l, 'h, 'g, 'p, 'r> Arg<'n, 'l, 'h, 'g, 'p, 'r> { /// `.min_values(2)`, and this argument would be satisfied if the user provided, 2 or more /// values. /// - /// **NOTE:** The argument *must* have `.multiple(true)` or `...` to use this setting. + /// **NOTE:** This implicity sets `.multiple(true)` + /// + /// **NOTE:** `qty` must be > 0 /// /// **NOTE:** `qty` *must* be > 0. If you wish to have an argument with 0 or more values prefer /// two seperate arguments (a flag, and an option with multiple values). @@ -714,7 +730,12 @@ impl<'n, 'l, 'h, 'g, 'p, 'r> Arg<'n, 'l, 'h, 'g, 'p, 'r> { /// .min_values(2) /// # ).get_matches(); pub fn min_values(mut self, qty: u8) -> Arg<'n, 'l, 'h, 'g, 'p, 'r> { + if qty < 1 { + panic!("Arguments with min_values(qty) qty must be > 0. Prefer flags for arguments \ + with 0 values."); + } self.min_vals = Some(qty); + self.multiple = true; self } @@ -727,6 +748,10 @@ impl<'n, 'l, 'h, 'g, 'p, 'r> Arg<'n, 'l, 'h, 'g, 'p, 'r> { /// be aware that the number of "names" you set for the values, will be the *exact* number of /// values required to satisfy this argument /// + /// **NOTE:** Does *not* require `.multiple(true)` to be set. Setting `.multiple(true)` would + /// allow `-f -f ` where as *not* setting + /// `.multiple(true)` would only allow one occurrence of this argument. + /// /// # Example /// /// ```no_run