From 630a4894b39c6ce1c6f522e0df4d678a3901197f Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 28 Jul 2022 16:48:47 -0500 Subject: [PATCH 01/11] fix(error): Ensure empty possible values isn't shown --- src/error/mod.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/error/mod.rs b/src/error/mod.rs index 648b894e1aa..68956149727 100644 --- a/src/error/mod.rs +++ b/src/error/mod.rs @@ -545,15 +545,17 @@ impl Error { let possible_values = self.get_context(ContextKind::ValidValue); if let Some(ContextValue::Strings(possible_values)) = possible_values { - c.none("\n\t[possible values: "); - if let Some((last, elements)) = possible_values.split_last() { - for v in elements { - c.good(escape(v)); - c.none(", "); + if !possible_values.is_empty() { + c.none("\n\t[possible values: "); + if let Some((last, elements)) = possible_values.split_last() { + for v in elements { + c.good(escape(v)); + c.none(", "); + } + c.good(escape(last)); } - c.good(escape(last)); + c.none("]"); } - c.none("]"); } let suggestion = self.get_context(ContextKind::SuggestedValue); From 5444b60361292122fba34b65a6590df178313001 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 28 Jul 2022 16:51:41 -0500 Subject: [PATCH 02/11] test: Verify max_values cases for 0 values --- tests/builder/multiple_values.rs | 41 ++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index 197f98ca1c4..877a314e740 100644 --- a/tests/builder/multiple_values.rs +++ b/tests/builder/multiple_values.rs @@ -341,6 +341,47 @@ fn option_max_less() { ); } +#[test] +fn option_max_zero() { + let m = Command::new("multiple_values") + .arg( + Arg::new("option") + .short('o') + .help("multiple options") + .max_values(3) + .action(ArgAction::Append), + ) + .try_get_matches_from(vec!["", "-o"]); + + assert!(m.is_err()); + assert_eq!(m.unwrap_err().kind(), ErrorKind::InvalidValue); +} + +#[test] +fn option_max_zero_eq() { + let m = Command::new("multiple_values") + .arg( + Arg::new("option") + .short('o') + .help("multiple options") + .max_values(3) + .action(ArgAction::Append), + ) + .try_get_matches_from(vec!["", "-o="]); + + assert!(m.is_ok(), "{}", m.unwrap_err()); + let m = m.unwrap(); + + assert!(m.contains_id("option")); + assert_eq!( + m.get_many::("option") + .unwrap() + .map(|v| v.as_str()) + .collect::>(), + [""] + ); +} + #[test] fn option_max_more() { let m = Command::new("multiple_values") From 4f756c483fddfd92e860b6ede339c08d873b3424 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 28 Jul 2022 16:13:40 -0500 Subject: [PATCH 03/11] fix(derive): Make consistent with `arg!` This has the downside of a regression in `--help`. We expect to fix that soon-ish. --- clap_derive/src/derives/args.rs | 1 - tests/derive/options.rs | 3 +-- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index fecb72f4789..8942678bd86 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -260,7 +260,6 @@ pub fn gen_augment( .value_name(#value_name) .min_values(0) .max_values(1) - .multiple_values(false) #value_parser #action }, diff --git a/tests/derive/options.rs b/tests/derive/options.rs index c15cf95057c..eb2b433be96 100644 --- a/tests/derive/options.rs +++ b/tests/derive/options.rs @@ -271,8 +271,7 @@ fn option_option_type_help() { arg: Option>, } let help = utils::get_help::(); - assert!(help.contains("--arg []")); - assert!(!help.contains("--arg []...")); + assert!(help.contains("--arg [...]")); } #[test] From ae803e1410dc624b8918ab95545e48dbc186eb4b Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 28 Jul 2022 16:16:14 -0500 Subject: [PATCH 04/11] test: Port macro/derive test to builder Making sure we cover the expected experience from multiple perspectives --- tests/builder/multiple_values.rs | 43 ++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index 877a314e740..06c0878f24d 100644 --- a/tests/builder/multiple_values.rs +++ b/tests/builder/multiple_values.rs @@ -400,6 +400,49 @@ fn option_max_more() { assert_eq!(m.unwrap_err().kind(), ErrorKind::TooManyValues); } +#[test] +fn optional_value() { + let mut cmd = Command::new("test").arg( + Arg::new("port") + .short('p') + .value_name("NUM") + .min_values(0) + .max_values(1), + ); + + let r = cmd.try_get_matches_from_mut(["test", "-p42"]); + assert!(r.is_ok(), "{}", r.unwrap_err()); + let m = r.unwrap(); + assert!(m.contains_id("port")); + assert_eq!(m.get_one::("port").unwrap(), "42"); + + let r = cmd.try_get_matches_from_mut(["test", "-p"]); + assert!(r.is_ok(), "{}", r.unwrap_err()); + let m = r.unwrap(); + assert!(m.contains_id("port")); + assert!(m.get_one::("port").is_none()); + + let r = cmd.try_get_matches_from_mut(["test", "-p", "24", "-p", "42"]); + assert!(r.is_ok(), "{}", r.unwrap_err()); + let m = r.unwrap(); + assert!(m.contains_id("port")); + assert_eq!(m.get_one::("port").unwrap(), "42"); + + let mut help = Vec::new(); + cmd.write_help(&mut help).unwrap(); + const HELP: &str = "\ +test + +USAGE: + test [OPTIONS] + +OPTIONS: + -p [...] + -h, --help Print help information +"; + snapbox::assert_eq(HELP, help); +} + #[test] fn positional() { let m = Command::new("multiple_values") From b4dfdcea15cfea06508ee6715b458f41879b4726 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 27 Jul 2022 13:28:10 -0500 Subject: [PATCH 05/11] fix!: Change number_of_values to be per occurrence --- CHANGELOG.md | 1 + src/parser/arg_matcher.rs | 14 ++++------ src/parser/matches/matched_arg.rs | 1 - src/parser/validator.rs | 46 +++++++++++++------------------ tests/builder/multiple_values.rs | 6 ++-- 5 files changed, 30 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 95f3adad7d2..c3280701233 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `arg!` now sets `ArgAction::SetTrue`, `ArgAction::Count`, `ArgAction::Set`, or `ArgAction::Append` as appropriate (#3795) - Default actions are now `Set` and `SetTrue` - Removed `PartialEq` and `Eq` from `Command` +- `Arg::number_of_values` now applies per occurrence rather than the average across all occurrences - `number_of_values(0)` no longer implies `takes_value(true).multiple_values(true)` - `number_of_values(1)` no longer implies `multiple_values(true)` - `ArgAction::SetTrue` and `ArgAction::SetFalse` now prioritize `Arg::default_missing_value` over their standard behavior diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index bf306b26946..d644a7a68c2 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -11,7 +11,6 @@ use crate::parser::Identifier; use crate::parser::PendingArg; use crate::parser::{ArgMatches, MatchedArg, SubCommand, ValueSource}; use crate::util::Id; -use crate::ArgAction; use crate::INTERNAL_ERROR_MSG; #[derive(Debug, Default)] @@ -217,13 +216,12 @@ impl ArgMatcher { ); if current_num == 0 { true - } else if let Some(num) = o.num_vals { - debug!("ArgMatcher::needs_more_vals: num_vals...{}", num); - if matches!(o.get_action(), ArgAction::Append) && !o.is_positional() { - (current_num % num) != 0 - } else { - num != current_num - } + } else if let Some(expected) = o.num_vals { + debug!( + "ArgMatcher::needs_more_vals: expected={}, actual={}", + expected, num_pending + ); + expected != num_pending } else if let Some(num) = o.max_vals { debug!("ArgMatcher::needs_more_vals: max_vals...{}", num); current_num < num diff --git a/src/parser/matches/matched_arg.rs b/src/parser/matches/matched_arg.rs index b66b5616c9d..d031ebd7ef1 100644 --- a/src/parser/matches/matched_arg.rs +++ b/src/parser/matches/matched_arg.rs @@ -89,7 +89,6 @@ impl MatchedArg { self.indices.push(index) } - #[cfg(feature = "unstable-grouped")] pub(crate) fn vals(&self) -> Iter> { self.vals.iter() } diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 7e58cda7064..549cdff3ebc 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -6,7 +6,6 @@ use crate::output::Usage; use crate::parser::{ArgMatcher, MatchedArg, ParseState}; use crate::util::ChildGraph; use crate::util::Id; -use crate::ArgAction; use crate::{INTERNAL_ERROR_MSG, INVALID_UTF8}; pub(crate) struct Validator<'help, 'cmd> { @@ -249,32 +248,25 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { fn validate_arg_num_vals(&self, a: &Arg, ma: &MatchedArg) -> ClapResult<()> { debug!("Validator::validate_arg_num_vals"); - if let Some(num) = a.num_vals { - let total_num = ma.num_vals(); - debug!( - "Validator::validate_arg_num_vals: num_vals={}, actual={}", - num, total_num - ); - let should_err = if matches!(a.get_action(), ArgAction::Append) && !a.is_positional() { - total_num % num != 0 - } else { - num != total_num - }; - if should_err { - debug!("Validator::validate_arg_num_vals: Sending error WrongNumberOfValues"); - return Err(Error::wrong_number_of_values( - self.cmd, - a.to_string(), - num, - if matches!(a.get_action(), ArgAction::Append) && !a.is_positional() { - total_num % num - } else { - total_num - }, - Usage::new(self.cmd) - .required(&self.required) - .create_usage_with_title(&[]), - )); + if let Some(expected) = a.num_vals { + for (_i, group) in ma.vals().enumerate() { + let actual = group.len(); + debug!( + "Validator::validate_arg_num_vals: group={}, num_vals={}, actual={}", + _i, expected, actual + ); + if expected != actual { + debug!("Validator::validate_arg_num_vals: Sending error WrongNumberOfValues"); + return Err(Error::wrong_number_of_values( + self.cmd, + a.to_string(), + expected, + actual, + Usage::new(self.cmd) + .required(&self.required) + .create_usage_with_title(&[]), + )); + } } } if let Some(num) = a.max_vals { diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index 06c0878f24d..080a06136b0 100644 --- a/tests/builder/multiple_values.rs +++ b/tests/builder/multiple_values.rs @@ -93,7 +93,9 @@ fn option_exact_exact() { .number_of_values(3) .action(ArgAction::Append), ) - .try_get_matches_from(vec!["", "-o", "val1", "-o", "val2", "-o", "val3"]); + .try_get_matches_from(vec![ + "", "-o", "val1", "val2", "val3", "-o", "val4", "val5", "val6", + ]); assert!(m.is_ok(), "{}", m.unwrap_err()); let m = m.unwrap(); @@ -104,7 +106,7 @@ fn option_exact_exact() { .unwrap() .map(|v| v.as_str()) .collect::>(), - ["val1", "val2", "val3"] + ["val1", "val2", "val3", "val4", "val5", "val6"] ); } From 41535d5c46bf2e2af9f0c4f442b3ab32d8b30050 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Tue, 26 Jul 2022 16:28:02 -0500 Subject: [PATCH 06/11] feat: Extend `number_of_values` to support min/max per occurrence --- CHANGELOG.md | 1 + clap_complete/src/shells/zsh.rs | 2 +- src/builder/arg.rs | 175 ++++++++++++++++---- src/builder/debug_asserts.rs | 31 +++- src/builder/mod.rs | 18 ++- src/builder/range.rs | 267 +++++++++++++++++++++++++++++++ src/output/help.rs | 3 +- src/parser/arg_matcher.rs | 2 +- src/parser/validator.rs | 50 ++++-- tests/builder/help.rs | 2 +- tests/builder/multiple_values.rs | 3 +- 11 files changed, 497 insertions(+), 57 deletions(-) create mode 100644 src/builder/range.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index c3280701233..8318c648e2f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Features +- `Arg::number_of_values` now accepts ranges, allowing setting both the minimum and maximum number of values per occurrence - *(help)* Show `PossibleValue::help` in long help (`--help`) (#3312) ### Fixes diff --git a/clap_complete/src/shells/zsh.rs b/clap_complete/src/shells/zsh.rs index ccc527fae23..b7129e436d0 100644 --- a/clap_complete/src/shells/zsh.rs +++ b/clap_complete/src/shells/zsh.rs @@ -462,7 +462,7 @@ fn write_opts_of(p: &Command, p_global: Option<&Command>) -> String { None => format!(":{}: ", vn), }; let vc = match o.get_num_vals() { - Some(num_vals) => vc.repeat(num_vals), + Some(num_vals) => vc.repeat(num_vals.min_values()), None => vc, }; diff --git a/src/builder/arg.rs b/src/builder/arg.rs index 6388fca851b..a862abab1b6 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -14,6 +14,7 @@ use std::{env, ffi::OsString}; // Internal use super::{ArgFlags, ArgSettings}; use crate::builder::ArgPredicate; +use crate::builder::ValuesRange; use crate::util::{Id, Key}; use crate::ArgAction; use crate::PossibleValue; @@ -72,7 +73,7 @@ pub struct Arg<'help> { pub(crate) short_aliases: Vec<(char, bool)>, // (name, visible) pub(crate) disp_ord: Option, pub(crate) val_names: Vec<&'help str>, - pub(crate) num_vals: Option, + pub(crate) num_vals: Option, pub(crate) max_vals: Option, pub(crate) min_vals: Option, pub(crate) val_delim: Option, @@ -1080,45 +1081,160 @@ impl<'help> Arg<'help> { } } - /// The number of values allowed for this argument. + /// Specifies the number of values allowed per occurrence of this argument /// - /// For example, if you had a - /// `-f ` argument where you wanted exactly 3 'files' you would set - /// `.number_of_values(3)`, and this argument wouldn't be satisfied unless the user provided - /// 3 and only 3 values. + /// For example, if you had a `-f ` argument where you wanted exactly 3 'files' you would + /// set `.number_of_values(3)`, and this argument wouldn't be satisfied unless the user + /// provided 3 and only 3 values. /// - /// **NOTE:** implicitly sets [`Arg::action(ArgAction::Set)`] and [`Arg::multiple_values(true)`]. + /// **NOTE:** Users may specify values for arguments in any of the following methods + /// + /// - Using a space such as `-o value` or `--option value` + /// - Using an equals and no space such as `-o=value` or `--option=value` + /// - Use a short and no space such as `-ovalue` + /// + /// **WARNING:** + /// + /// Setting a variable number of values (e.g. `..10`) for an argument without + /// other details can be dangerous in some circumstances. Because multiple values are + /// allowed, `--option val1 val2 val3` is perfectly valid. Be careful when designing a CLI + /// where **positional arguments** or **subcommands** are *also* expected as `clap` will continue + /// parsing *values* until one of the following happens: + /// + /// - It reaches the maximum number of values + /// - It reaches a specific number of values + /// - It finds another flag or option (i.e. something that starts with a `-`) + /// - It reaches the [`Arg::value_terminator`] if set + /// + /// Alternatively, + /// - Require a delimiter between values with [Arg::require_value_delimiter] + /// - Require a flag occurrence per value with [`ArgAction::Append`] + /// - Require positional arguments to appear after `--` with [`Arg::last`] /// /// # Examples /// + /// Option: /// ```rust /// # use clap::{Command, Arg}; - /// Arg::new("file") - /// .short('f') - /// .number_of_values(3); + /// let m = Command::new("prog") + /// .arg(Arg::new("mode") + /// .long("mode") + /// .number_of_values(1)) + /// .get_matches_from(vec![ + /// "prog", "--mode", "fast" + /// ]); + /// + /// assert_eq!(m.get_one::("mode").unwrap(), "fast"); /// ``` /// - /// Not supplying the correct number of values is an error + /// Flag/option hybrid (see also [default_missing_value][Arg::default_missing_value]) + /// ```rust + /// # use clap::{Command, Arg, ErrorKind, ArgAction}; + /// let cmd = Command::new("prog") + /// .arg(Arg::new("mode") + /// .long("mode") + /// .default_missing_value("slow") + /// .default_value("plaid") + /// .number_of_values(0..=1)); + /// + /// let m = cmd.clone() + /// .get_matches_from(vec![ + /// "prog", "--mode", "fast" + /// ]); + /// assert_eq!(m.get_one::("mode").unwrap(), "fast"); + /// + /// let m = cmd.clone() + /// .get_matches_from(vec![ + /// "prog", "--mode", + /// ]); + /// assert_eq!(m.get_one::("mode").unwrap(), "slow"); + /// + /// let m = cmd.clone() + /// .get_matches_from(vec![ + /// "prog", + /// ]); + /// assert_eq!(m.get_one::("mode").unwrap(), "plaid"); + /// ``` /// + /// Tuples /// ```rust /// # use clap::{Command, Arg, ErrorKind, ArgAction}; - /// let res = Command::new("prog") + /// let cmd = Command::new("prog") /// .arg(Arg::new("file") /// .action(ArgAction::Set) /// .number_of_values(2) - /// .short('F')) + /// .short('F')); + /// + /// let m = cmd.clone() + /// .get_matches_from(vec![ + /// "prog", "-F", "in-file", "out-file" + /// ]); + /// assert_eq!( + /// m.get_many::("file").unwrap_or_default().map(|v| v.as_str()).collect::>(), + /// vec!["in-file", "out-file"] + /// ); + /// + /// let res = cmd.clone() /// .try_get_matches_from(vec![ /// "prog", "-F", "file1" /// ]); - /// - /// assert!(res.is_err()); /// assert_eq!(res.unwrap_err().kind(), ErrorKind::WrongNumberOfValues); /// ``` + /// + /// A common mistake is to define an option which allows multiple values and a positional + /// argument. + /// ```rust + /// # use clap::{Command, Arg, ArgAction}; + /// let cmd = Command::new("prog") + /// .arg(Arg::new("file") + /// .action(ArgAction::Set) + /// .number_of_values(0..) + /// .short('F')) + /// .arg(Arg::new("word")); + /// + /// let m = cmd.clone().get_matches_from(vec![ + /// "prog", "-F", "file1", "file2", "file3", "word" + /// ]); + /// let files: Vec<_> = m.get_many::("file").unwrap().collect(); + /// assert_eq!(files, ["file1", "file2", "file3", "word"]); // wait...what?! + /// assert!(!m.contains_id("word")); // but we clearly used word! + /// + /// // but this works + /// let m = cmd.clone().get_matches_from(vec![ + /// "prog", "word", "-F", "file1", "file2", "file3", + /// ]); + /// let files: Vec<_> = m.get_many::("file").unwrap().collect(); + /// assert_eq!(files, ["file1", "file2", "file3"]); + /// assert_eq!(m.get_one::("word").unwrap(), "word"); + /// ``` + /// The problem is `clap` doesn't know when to stop parsing values for "file". + /// + /// A solution for the example above is to limit how many values with a maximum, or specific + /// number, or to say [`ArgAction::Append`] is ok, but multiple values are not. + /// ```rust + /// # use clap::{Command, Arg, ArgAction}; + /// let m = Command::new("prog") + /// .arg(Arg::new("file") + /// .action(ArgAction::Append) + /// .short('F')) + /// .arg(Arg::new("word")) + /// .get_matches_from(vec![ + /// "prog", "-F", "file1", "-F", "file2", "-F", "file3", "word" + /// ]); + /// + /// let files: Vec<_> = m.get_many::("file").unwrap().collect(); + /// assert_eq!(files, ["file1", "file2", "file3"]); + /// assert_eq!(m.get_one::("word").unwrap(), "word"); + /// ``` + /// [`Arg::value_delimiter(char)`]: Arg::value_delimiter() + /// [multiple values]: Arg::multiple_values #[inline] #[must_use] - pub fn number_of_values(mut self, qty: usize) -> Self { + pub fn number_of_values(mut self, qty: impl Into) -> Self { + let qty = qty.into(); self.num_vals = Some(qty); - self.takes_value(qty != 0).multiple_values(1 < qty) + self.takes_value(qty.takes_values()) + .multiple_values(qty.is_multiple()) } /// The *maximum* number of values are for this argument. @@ -4062,7 +4178,7 @@ impl<'help> Arg<'help> { /// Get the number of values for this argument. #[inline] - pub fn get_num_vals(&self) -> Option { + pub fn get_num_vals(&self) -> Option { self.num_vals } @@ -4330,13 +4446,9 @@ impl<'help> Arg<'help> { } let val_names_len = self.val_names.len(); - if val_names_len > 1 { self.settings.set(ArgSettings::MultipleValues); - - if self.num_vals.is_none() { - self.num_vals = Some(val_names_len); - } + self.num_vals.get_or_insert(val_names_len.into()); } } @@ -4435,7 +4547,8 @@ impl<'help> Display for Arg<'help> { write!(f, "-{}", s)?; } let mut need_closing_bracket = false; - let is_optional_val = self.get_min_vals() == Some(0); + let is_optional_val = self.get_min_vals() == Some(0) + || self.get_num_vals().map(|r| r.min_values()) == Some(0); if self.is_positional() { if is_optional_val { let sep = "["; @@ -4552,10 +4665,17 @@ pub(crate) fn render_arg_val(arg: &Arg) -> String { let mut extra_values = false; debug_assert!(arg.is_takes_value_set()); - let num_vals = arg.num_vals.unwrap_or(1); + let num_vals = arg.num_vals.unwrap_or_else(|| { + if arg.is_multiple_values_set() { + (1..).into() + } else { + 1.into() + } + }); if val_names.len() == 1 { let arg_name = format!("<{}>", val_names[0]); - for n in 1..=num_vals { + let min = num_vals.min_values().max(1); + for n in 1..=min { if n != 1 { rendered.push_str(delim); } @@ -4571,8 +4691,9 @@ pub(crate) fn render_arg_val(arg: &Arg) -> String { } rendered.push_str(&arg_name); } - extra_values |= val_names.len() < num_vals; + extra_values |= val_names.len() < num_vals.max_values(); } + extra_values |= !num_vals.is_fixed(); if arg.is_positional() { if matches!(*arg.get_action(), ArgAction::Append) { extra_values = true; diff --git a/src/builder/debug_asserts.rs b/src/builder/debug_asserts.rs index 4bb3f26b8fb..552000d93ab 100644 --- a/src/builder/debug_asserts.rs +++ b/src/builder/debug_asserts.rs @@ -687,15 +687,32 @@ fn assert_arg(arg: &Arg) { ); } - let num_vals = arg.get_num_vals().unwrap_or(usize::MAX); - let num_val_names = arg.get_value_names().unwrap_or(&[]).len(); - if num_vals < num_val_names { - panic!( - "Argument {}: Too many value names ({}) compared to number_of_values ({})", - arg.name, num_val_names, num_vals + if let Some(num_vals) = arg.get_num_vals() { + // This can be the cause of later asserts, so put this first + let num_val_names = arg.get_value_names().unwrap_or(&[]).len(); + if num_vals.max_values() < num_val_names { + panic!( + "Argument {}: Too many value names ({}) compared to number_of_values ({})", + arg.name, num_val_names, num_vals + ); + } + + assert_eq!( + num_vals.takes_values(), + arg.is_takes_value_set(), + "Argument {}: mismatch between `number_of_values` ({}) and `takes_value`", + arg.name, + num_vals, + ); + assert_eq!( + num_vals.is_multiple(), + arg.is_multiple_values_set(), + "Argument {}: mismatch between `number_of_values` ({}) and `multiple_values`", + arg.name, + num_vals, ); } - if arg.get_num_vals() == Some(1) { + if arg.get_num_vals() == Some(1.into()) { assert!( !arg.is_multiple_values_set(), "Argument {}: mismatch between `number_of_values` and `multiple_values`", diff --git a/src/builder/mod.rs b/src/builder/mod.rs index f66fb48532f..2ee326d9cc6 100644 --- a/src/builder/mod.rs +++ b/src/builder/mod.rs @@ -8,6 +8,7 @@ mod arg_predicate; mod arg_settings; mod command; mod possible_value; +mod range; mod value_hint; mod value_parser; @@ -22,15 +23,8 @@ pub use arg::Arg; pub use arg_group::ArgGroup; pub use command::Command; pub use possible_value::PossibleValue; +pub use range::ValuesRange; pub use value_hint::ValueHint; -pub use value_parser::PossibleValuesParser; -pub use value_parser::RangedI64ValueParser; -pub use value_parser::RangedU64ValueParser; -pub use value_parser::StringValueParser; -pub use value_parser::TypedValueParser; -pub use value_parser::ValueParser; -pub use value_parser::ValueParserFactory; -pub use value_parser::_AnonymousValueParser; pub use value_parser::_AutoValueParser; pub use value_parser::via_prelude; pub use value_parser::BoolValueParser; @@ -40,6 +34,14 @@ pub use value_parser::FalseyValueParser; pub use value_parser::NonEmptyStringValueParser; pub use value_parser::OsStringValueParser; pub use value_parser::PathBufValueParser; +pub use value_parser::PossibleValuesParser; +pub use value_parser::RangedI64ValueParser; +pub use value_parser::RangedU64ValueParser; +pub use value_parser::StringValueParser; +pub use value_parser::TypedValueParser; +pub use value_parser::ValueParser; +pub use value_parser::ValueParserFactory; +pub use value_parser::_AnonymousValueParser; pub(crate) use action::CountType; pub(crate) use arg::render_arg_val; diff --git a/src/builder/range.rs b/src/builder/range.rs new file mode 100644 index 00000000000..c634f068e4d --- /dev/null +++ b/src/builder/range.rs @@ -0,0 +1,267 @@ +/// Values per occurrence for an argument +#[derive(Copy, Clone, PartialEq, Eq, Hash)] +pub struct ValuesRange { + start_inclusive: usize, + end_inclusive: usize, +} + +impl ValuesRange { + /// Create a range + /// + /// # Panics + /// + /// If the end is less than the start + /// + /// # Examples + /// + /// ``` + /// # use clap::builder::ValuesRange; + /// let range = ValuesRange::new(5); + /// let range = ValuesRange::new(5..10); + /// let range = ValuesRange::new(5..=10); + /// let range = ValuesRange::new(5..); + /// let range = ValuesRange::new(..10); + /// let range = ValuesRange::new(..=10); + /// ``` + /// + /// While this will panic: + /// ```should_panic + /// # use clap::builder::ValuesRange; + /// let range = ValuesRange::new(10..5); // Panics! + /// ``` + pub fn new(range: impl Into) -> Self { + range.into() + } + + pub(crate) fn raw(start_inclusive: usize, end_inclusive: usize) -> Self { + debug_assert!(start_inclusive <= end_inclusive); + Self { + start_inclusive, + end_inclusive, + } + } + + /// Fewest number of values the argument accepts + pub fn min_values(&self) -> usize { + self.start_inclusive + } + + /// Most number of values the argument accepts + pub fn max_values(&self) -> usize { + self.end_inclusive + } + + /// Report whether the argument takes any values (ie is a flag) + /// + /// # Examples + /// + /// ``` + /// # use clap::builder::ValuesRange; + /// let range = ValuesRange::new(5); + /// assert!(range.takes_values()); + /// + /// let range = ValuesRange::new(0); + /// assert!(!range.takes_values()); + /// ``` + pub fn takes_values(&self) -> bool { + self.end_inclusive != 0 + } + + pub(crate) fn is_fixed(&self) -> bool { + self.start_inclusive == self.end_inclusive + } + + pub(crate) fn is_multiple(&self) -> bool { + self.start_inclusive != self.end_inclusive || 1 < self.start_inclusive + } + + pub(crate) fn num_values(&self) -> Option { + self.is_fixed().then(|| self.start_inclusive) + } + + pub(crate) fn accepts_more(&self, current: usize) -> bool { + current < self.end_inclusive + } +} + +impl std::ops::RangeBounds for ValuesRange { + fn start_bound(&self) -> std::ops::Bound<&usize> { + std::ops::Bound::Included(&self.start_inclusive) + } + + fn end_bound(&self) -> std::ops::Bound<&usize> { + std::ops::Bound::Included(&self.end_inclusive) + } +} + +impl Default for ValuesRange { + fn default() -> Self { + 0.into() + } +} + +impl From for ValuesRange { + fn from(fixed: usize) -> Self { + (fixed..=fixed).into() + } +} + +impl From> for ValuesRange { + fn from(range: std::ops::Range) -> Self { + let start_inclusive = range.start; + let end_inclusive = range.end.saturating_sub(1); + Self::raw(start_inclusive, end_inclusive) + } +} + +impl From for ValuesRange { + fn from(_: std::ops::RangeFull) -> Self { + let start_inclusive = 0; + let end_inclusive = usize::MAX; + Self::raw(start_inclusive, end_inclusive) + } +} + +impl From> for ValuesRange { + fn from(range: std::ops::RangeFrom) -> Self { + let start_inclusive = range.start; + let end_inclusive = usize::MAX; + Self::raw(start_inclusive, end_inclusive) + } +} + +impl From> for ValuesRange { + fn from(range: std::ops::RangeTo) -> Self { + let start_inclusive = 0; + let end_inclusive = range.end.saturating_sub(1); + Self::raw(start_inclusive, end_inclusive) + } +} + +impl From> for ValuesRange { + fn from(range: std::ops::RangeInclusive) -> Self { + let start_inclusive = *range.start(); + let end_inclusive = *range.end(); + Self::raw(start_inclusive, end_inclusive) + } +} + +impl From> for ValuesRange { + fn from(range: std::ops::RangeToInclusive) -> Self { + let start_inclusive = 0; + let end_inclusive = range.end; + Self::raw(start_inclusive, end_inclusive) + } +} + +impl std::fmt::Display for ValuesRange { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + self.start_inclusive.fmt(f)?; + if !self.is_fixed() { + "..=".fmt(f)?; + self.end_inclusive.fmt(f)?; + } + Ok(()) + } +} + +impl std::fmt::Debug for ValuesRange { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(f, "{}", self) + } +} + +#[cfg(test)] +mod test { + use super::*; + + use std::ops::RangeBounds; + + #[test] + fn from_fixed() { + let range: ValuesRange = 5.into(); + assert_eq!(range.start_bound(), std::ops::Bound::Included(&5)); + assert_eq!(range.end_bound(), std::ops::Bound::Included(&5)); + assert!(range.is_fixed()); + assert!(range.is_multiple()); + assert_eq!(range.num_values(), Some(5)); + assert!(range.takes_values()); + } + + #[test] + fn from_fixed_empty() { + let range: ValuesRange = 0.into(); + assert_eq!(range.start_bound(), std::ops::Bound::Included(&0)); + assert_eq!(range.end_bound(), std::ops::Bound::Included(&0)); + assert!(range.is_fixed()); + assert!(!range.is_multiple()); + assert_eq!(range.num_values(), Some(0)); + assert!(!range.takes_values()); + } + + #[test] + fn from_range() { + let range: ValuesRange = (5..10).into(); + assert_eq!(range.start_bound(), std::ops::Bound::Included(&5)); + assert_eq!(range.end_bound(), std::ops::Bound::Included(&9)); + assert!(!range.is_fixed()); + assert!(range.is_multiple()); + assert_eq!(range.num_values(), None); + assert!(range.takes_values()); + } + + #[test] + fn from_range_inclusive() { + let range: ValuesRange = (5..=10).into(); + assert_eq!(range.start_bound(), std::ops::Bound::Included(&5)); + assert_eq!(range.end_bound(), std::ops::Bound::Included(&10)); + assert!(!range.is_fixed()); + assert!(range.is_multiple()); + assert_eq!(range.num_values(), None); + assert!(range.takes_values()); + } + + #[test] + fn from_range_full() { + let range: ValuesRange = (..).into(); + assert_eq!(range.start_bound(), std::ops::Bound::Included(&0)); + assert_eq!(range.end_bound(), std::ops::Bound::Included(&usize::MAX)); + assert!(!range.is_fixed()); + assert!(range.is_multiple()); + assert_eq!(range.num_values(), None); + assert!(range.takes_values()); + } + + #[test] + fn from_range_from() { + let range: ValuesRange = (5..).into(); + assert_eq!(range.start_bound(), std::ops::Bound::Included(&5)); + assert_eq!(range.end_bound(), std::ops::Bound::Included(&usize::MAX)); + assert!(!range.is_fixed()); + assert!(range.is_multiple()); + assert_eq!(range.num_values(), None); + assert!(range.takes_values()); + } + + #[test] + fn from_range_to() { + let range: ValuesRange = (..10).into(); + assert_eq!(range.start_bound(), std::ops::Bound::Included(&0)); + assert_eq!(range.end_bound(), std::ops::Bound::Included(&9)); + assert!(!range.is_fixed()); + assert!(range.is_multiple()); + assert_eq!(range.num_values(), None); + assert!(range.takes_values()); + } + + #[test] + fn from_range_to_inclusive() { + let range: ValuesRange = (..=10).into(); + assert_eq!(range.start_bound(), std::ops::Bound::Included(&0)); + assert_eq!(range.end_bound(), std::ops::Bound::Included(&10)); + assert!(!range.is_fixed()); + assert!(range.is_multiple()); + assert_eq!(range.num_values(), None); + assert!(range.takes_values()); + } +} diff --git a/src/output/help.rs b/src/output/help.rs index f3228831e97..fff6c78499a 100644 --- a/src/output/help.rs +++ b/src/output/help.rs @@ -296,7 +296,8 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> { debug!("Help::val: arg={}", arg.name); let mut need_closing_bracket = false; if arg.is_takes_value_set() && !arg.is_positional() { - let is_optional_val = arg.get_min_vals() == Some(0); + let is_optional_val = arg.get_min_vals() == Some(0) + || arg.get_num_vals().map(|r| r.min_values()) == Some(0); let sep = if arg.is_require_equals_set() { if is_optional_val { need_closing_bracket = true; diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index d644a7a68c2..d8812b63773 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -221,7 +221,7 @@ impl ArgMatcher { "ArgMatcher::needs_more_vals: expected={}, actual={}", expected, num_pending ); - expected != num_pending + expected.accepts_more(num_pending) } else if let Some(num) = o.max_vals { debug!("ArgMatcher::needs_more_vals: max_vals...{}", num); current_num < num diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 549cdff3ebc..69ad68b479a 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -33,7 +33,9 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { let o = &self.cmd[&a]; let should_err = if let Some(v) = matcher.args.get(&o.id) { - v.all_val_groups_empty() && o.get_min_vals() != Some(0) + v.all_val_groups_empty() + && (o.get_min_vals() != Some(0) + && o.get_num_vals().map(|r| r.min_values()) != Some(0)) } else { true }; @@ -248,6 +250,8 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { fn validate_arg_num_vals(&self, a: &Arg, ma: &MatchedArg) -> ClapResult<()> { debug!("Validator::validate_arg_num_vals"); + let mut min_vals_zero = false; + if let Some(expected) = a.num_vals { for (_i, group) in ma.vals().enumerate() { let actual = group.len(); @@ -255,17 +259,47 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { "Validator::validate_arg_num_vals: group={}, num_vals={}, actual={}", _i, expected, actual ); - if expected != actual { - debug!("Validator::validate_arg_num_vals: Sending error WrongNumberOfValues"); - return Err(Error::wrong_number_of_values( + min_vals_zero |= expected.min_values() == 0; + if let Some(expected) = expected.num_values() { + if expected != actual { + debug!( + "Validator::validate_arg_num_vals: Sending error WrongNumberOfValues" + ); + return Err(Error::wrong_number_of_values( + self.cmd, + a.to_string(), + expected, + actual, + Usage::new(self.cmd) + .required(&self.required) + .create_usage_with_title(&[]), + )); + } + } else if actual < expected.min_values() { + return Err(Error::too_few_values( self.cmd, a.to_string(), - expected, + expected.min_values(), actual, Usage::new(self.cmd) .required(&self.required) .create_usage_with_title(&[]), )); + } else if expected.max_values() < actual { + debug!("Validator::validate_arg_num_vals: Sending error TooManyValues"); + return Err(Error::too_many_values( + self.cmd, + ma.raw_vals_flatten() + .last() + .expect(INTERNAL_ERROR_MSG) + .to_str() + .expect(INVALID_UTF8) + .to_string(), + a.to_string(), + Usage::new(self.cmd) + .required(&self.required) + .create_usage_with_title(&[]), + )); } } } @@ -288,7 +322,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { )); } } - let min_vals_zero = if let Some(num) = a.get_min_vals() { + if let Some(num) = a.get_min_vals() { debug!("Validator::validate_arg_num_vals: min_vals set: {}", num); if ma.num_vals() < num && num != 0 { debug!("Validator::validate_arg_num_vals: Sending error TooFewValues"); @@ -302,9 +336,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { .create_usage_with_title(&[]), )); } - num == 0 - } else { - false + min_vals_zero |= num == 0; }; // Issue 665 (https://github.com/clap-rs/clap/issues/665) // Issue 1105 (https://github.com/clap-rs/clap/issues/1105) diff --git a/tests/builder/help.rs b/tests/builder/help.rs index 64d08b0b63e..af61d56e0aa 100644 --- a/tests/builder/help.rs +++ b/tests/builder/help.rs @@ -1044,7 +1044,7 @@ OPTIONS: } #[test] -#[should_panic = "Argument option: mismatch between `number_of_values` and `multiple_values`"] +#[should_panic = "Argument option: mismatch between `number_of_values` (1) and `multiple_values`"] fn number_of_values_conflicts_with_multiple_values() { Command::new("ctest") .version("0.1") diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index 080a06136b0..e2df2f363de 100644 --- a/tests/builder/multiple_values.rs +++ b/tests/builder/multiple_values.rs @@ -408,8 +408,7 @@ fn optional_value() { Arg::new("port") .short('p') .value_name("NUM") - .min_values(0) - .max_values(1), + .number_of_values(0..=1), ); let r = cmd.try_get_matches_from_mut(["test", "-p42"]); From ab8ef4666309615b3b2923d66ead32709e7d51aa Mon Sep 17 00:00:00 2001 From: Ed Page Date: Wed, 27 Jul 2022 21:00:31 -0500 Subject: [PATCH 07/11] fix: arg!(--long [value]) to per occurrence values Before we did 0..=1 across all occurrences when what we really wanted was 0..=1 per occurrence. This makes it compatible with `ArgAction::Append`. --- CHANGELOG.md | 1 + src/macros.rs | 4 ++-- tests/builder/default_missing_vals.rs | 3 +-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8318c648e2f..66beab79ca8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Replaced `cmd.allow_invalid_for_utf8_external_subcommands` with `cmd.external_subcommand_value_parser` (#3733) - Changed the default type of `allow_external_subcommands` from `String` to `OsString` - `Arg::default_missing_value` now applies per occurrence rather than if a value is missing across all occurrences +- `arg!(--long [value])` to accept `0..=1` per occurrence rather than across all occurrences, making it safe to use with `ArgAction::Append` - *(assert)* Always enforce that version is specified when the `ArgAction::Version` is used - *(assert)* Add missing `#[track_caller]`s to make it easier to debug asserts - *(assert)* Ensure `overrides_with` IDs are valid diff --git a/src/macros.rs b/src/macros.rs index ae228713f41..cb3d6d2ee3b 100644 --- a/src/macros.rs +++ b/src/macros.rs @@ -355,7 +355,7 @@ macro_rules! arg_impl { if arg.get_long().is_none() && arg.get_short().is_none() { arg = arg.required(false); } else { - arg = arg.min_values(0).max_values(1); + arg = arg.number_of_values(0..=1); } let value_name = $crate::arg_impl! { @string $value_name }; @@ -387,7 +387,7 @@ macro_rules! arg_impl { if arg.get_long().is_none() && arg.get_short().is_none() { arg = arg.required(false); } else { - arg = arg.min_values(0).max_values(1); + arg = arg.number_of_values(0..=1); } let value_name = $crate::arg_impl! { @string $value_name }; diff --git a/tests/builder/default_missing_vals.rs b/tests/builder/default_missing_vals.rs index dad62f2e2bc..6f747c84d5b 100644 --- a/tests/builder/default_missing_vals.rs +++ b/tests/builder/default_missing_vals.rs @@ -148,8 +148,7 @@ fn default_missing_value_per_occurrence() { // assert no change to usual argument handling when adding default_missing_value() let r = Command::new("cmd") .arg( - arg!(o: -o ... "some opt") - .min_values(0) + arg!(o: -o [opt] ... "some opt") .default_value("default") .default_missing_value("default_missing"), ) From cf8e1fc319cf5af51bdfacbcf26ad4f026c08bb7 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 28 Jul 2022 12:35:49 -0500 Subject: [PATCH 08/11] fix(derive): Make `Option>` work per occurrence --- clap_derive/src/derives/args.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/clap_derive/src/derives/args.rs b/clap_derive/src/derives/args.rs index 8942678bd86..73d56f6e880 100644 --- a/clap_derive/src/derives/args.rs +++ b/clap_derive/src/derives/args.rs @@ -258,8 +258,7 @@ pub fn gen_augment( Ty::OptionOption => quote_spanned! { ty.span()=> .value_name(#value_name) - .min_values(0) - .max_values(1) + .number_of_values(0..=1) #value_parser #action }, From ccf35ff70cc4aa3da009693494761ae29d6b5fda Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 28 Jul 2022 17:13:41 -0500 Subject: [PATCH 09/11] fix!: Replace `min_values` (total) with `number_of_values` (per occurrence) --- CHANGELOG.md | 1 + clap_bench/benches/03_complex.rs | 4 +- src/builder/arg.rs | 98 ++++----------------------- src/error/kind.rs | 6 +- src/output/help.rs | 3 +- src/parser/arg_matcher.rs | 3 - src/parser/matches/arg_matches.rs | 2 +- src/parser/validator.rs | 66 +++++++----------- tests/builder/default_missing_vals.rs | 10 +-- tests/builder/help.rs | 26 +++---- tests/builder/multiple_values.rs | 40 +++++++---- tests/builder/opts.rs | 4 +- tests/builder/require.rs | 6 +- tests/builder/utils.rs | 8 +-- tests/derive/options.rs | 2 +- 15 files changed, 99 insertions(+), 180 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 66beab79ca8..b199bf6d1e9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,6 +23,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `Arg::number_of_values` now applies per occurrence rather than the average across all occurrences - `number_of_values(0)` no longer implies `takes_value(true).multiple_values(true)` - `number_of_values(1)` no longer implies `multiple_values(true)` +- Remove `Arg::min_values` (across all occurrences) with `Arg::number_of_values` (per occurrence) - `ArgAction::SetTrue` and `ArgAction::SetFalse` now prioritize `Arg::default_missing_value` over their standard behavior - *(help)* Make `DeriveDisplayOrder` the default and removed the setting. To sort help, set `next_display_order(None)` (#2808) - *(help)* Subcommand display order respects `Command::next_display_order` instead of `DeriveDisplayOrder` and using its own initial display order value (#2808) diff --git a/clap_bench/benches/03_complex.rs b/clap_bench/benches/03_complex.rs index a1dcebfd45b..c56d7372954 100644 --- a/clap_bench/benches/03_complex.rs +++ b/clap_bench/benches/03_complex.rs @@ -31,7 +31,7 @@ macro_rules! create_app { arg!( --multvalsmo "Tests multiple values, not mult occs" ).multiple_values(true).required(false).value_names(&["one", "two"]), - arg!(--minvals2 ... "Tests 2 min vals").min_values(2).multiple_values(true).required(false), + arg!(--minvals2 ... "Tests 2 min vals").number_of_values(2..).multiple_values(true).required(false), arg!(--maxvals3 ... "Tests 3 max vals").max_values(3).multiple_values(true).required(false), ]) .subcommand( @@ -124,7 +124,7 @@ pub fn build_from_builder(c: &mut Criterion) { .multiple_values(true) .action(ArgAction::Append) .help("Tests 2 min vals") - .min_values(2), + .number_of_values(2..), ) .arg( Arg::new("maxvals") diff --git a/src/builder/arg.rs b/src/builder/arg.rs index a862abab1b6..cdc03ad59b6 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -75,7 +75,6 @@ pub struct Arg<'help> { pub(crate) val_names: Vec<&'help str>, pub(crate) num_vals: Option, pub(crate) max_vals: Option, - pub(crate) min_vals: Option, pub(crate) val_delim: Option, pub(crate) default_vals: Vec<&'help OsStr>, pub(crate) default_vals_ifs: Vec<(Id, ArgPredicate<'help>, Option<&'help OsStr>)>, @@ -1294,70 +1293,6 @@ impl<'help> Arg<'help> { self.takes_value(true).multiple_values(true) } - /// The *minimum* number of values for this argument. - /// - /// For example, if you had a - /// `-f ` argument where you wanted at least 2 'files' you would set - /// `.min_values(2)`, and this argument would be satisfied if the user provided, 2 or more - /// values. - /// - /// **NOTE:** Passing a non-zero value is not the same as specifying [`Arg::required(true)`]. - /// This is due to min and max validation only being performed for present arguments, - /// marking them as required will thus perform validation and a min value of 1 - /// is unnecessary, ignored if not required. - /// - /// # Examples - /// - /// ```rust - /// # use clap::{Command, Arg}; - /// Arg::new("file") - /// .short('f') - /// .min_values(3); - /// ``` - /// - /// Supplying more than the minimum number of values is allowed - /// - /// ```rust - /// # use clap::{Command, Arg, ArgAction}; - /// let res = Command::new("prog") - /// .arg(Arg::new("file") - /// .action(ArgAction::Set) - /// .min_values(2) - /// .short('F')) - /// .try_get_matches_from(vec![ - /// "prog", "-F", "file1", "file2", "file3" - /// ]); - /// - /// assert!(res.is_ok()); - /// let m = res.unwrap(); - /// let files: Vec<_> = m.get_many::("file").unwrap().collect(); - /// assert_eq!(files, ["file1", "file2", "file3"]); - /// ``` - /// - /// Supplying less than the minimum number of values is an error - /// - /// ```rust - /// # use clap::{Command, Arg, ErrorKind, ArgAction}; - /// let res = Command::new("prog") - /// .arg(Arg::new("file") - /// .action(ArgAction::Set) - /// .min_values(2) - /// .short('F')) - /// .try_get_matches_from(vec![ - /// "prog", "-F", "file1" - /// ]); - /// - /// assert!(res.is_err()); - /// assert_eq!(res.unwrap_err().kind(), ErrorKind::TooFewValues); - /// ``` - /// [`Arg::required(true)`]: Arg::required() - #[inline] - #[must_use] - pub fn min_values(mut self, qty: usize) -> Self { - self.min_vals = Some(qty); - self.takes_value(true).multiple_values(true) - } - /// Placeholder for the argument's value in the help message / usage. /// /// This name is cosmetic only; the name is **not** used to access arguments. @@ -1856,8 +1791,7 @@ impl<'help> Arg<'help> { /// By default when /// one sets [`multiple_values(true)`] on an argument, clap will continue parsing values for that /// argument until it reaches another valid argument, or one of the other more specific settings - /// for multiple values is used (such as [`min_values`], [`max_values`] or - /// [`number_of_values`]). + /// for multiple values is used (such as [`max_values`] or [`number_of_values`]). /// /// **NOTE:** This setting only applies to [options] and [positional arguments] /// @@ -1897,7 +1831,6 @@ impl<'help> Arg<'help> { /// [options]: Arg::takes_value() /// [positional arguments]: Arg::index() /// [`multiple_values(true)`]: Arg::multiple_values() - /// [`min_values`]: Arg::min_values() /// [`number_of_values`]: Arg::number_of_values() /// [`max_values`]: Arg::max_values() #[inline] @@ -2047,9 +1980,10 @@ impl<'help> Arg<'help> { /// argument is a common example. By, supplying an default, such as `default_missing_value("always")`, /// the user can quickly just add `--color` to the command line to produce the desired color output. /// - /// **NOTE:** using this configuration option requires the use of the `.min_values(0)` and the - /// `.require_equals(true)` configuration option. These are required in order to unambiguously - /// determine what, if any, value was supplied for the argument. + /// **NOTE:** using this configuration option requires the use of the + /// [`.number_of_values(0..N)`][Arg::number_of_values] and the + /// [`.require_equals(true)`][Arg::require_equals] configuration option. These are required in + /// order to unambiguously determine what, if any, value was supplied for the argument. /// /// # Examples /// @@ -2062,7 +1996,7 @@ impl<'help> Arg<'help> { /// .value_name("WHEN") /// .value_parser(["always", "auto", "never"]) /// .default_value("auto") - /// .min_values(0) + /// .number_of_values(0..=1) /// .require_equals(true) /// .default_missing_value("always") /// .help("Specify WHEN to colorize output.") @@ -2099,7 +2033,7 @@ impl<'help> Arg<'help> { /// .arg(Arg::new("create").long("create") /// .value_name("BOOL") /// .value_parser(value_parser!(bool)) - /// .min_values(0) + /// .number_of_values(0..=1) /// .require_equals(true) /// .default_missing_value("true") /// ) @@ -4184,7 +4118,7 @@ impl<'help> Arg<'help> { #[inline] pub(crate) fn get_min_vals(&self) -> Option { - self.min_vals + self.get_num_vals().map(|r| r.min_values()) } /// Get the delimiter between multiple values @@ -4547,8 +4481,7 @@ impl<'help> Display for Arg<'help> { write!(f, "-{}", s)?; } let mut need_closing_bracket = false; - let is_optional_val = self.get_min_vals() == Some(0) - || self.get_num_vals().map(|r| r.min_values()) == Some(0); + let is_optional_val = self.get_min_vals() == Some(0); if self.is_positional() { if is_optional_val { let sep = "["; @@ -4611,7 +4544,6 @@ impl<'help> fmt::Debug for Arg<'help> { .field("val_names", &self.val_names) .field("num_vals", &self.num_vals) .field("max_vals", &self.max_vals) - .field("min_vals", &self.min_vals) .field("val_delim", &self.val_delim) .field("default_vals", &self.default_vals) .field("default_vals_ifs", &self.default_vals_ifs) @@ -4799,7 +4731,7 @@ mod test { let mut o = Arg::new("opt") .long("option") .action(ArgAction::Set) - .min_values(0); + .number_of_values(0..); o._build(); assert_eq!(o.to_string(), "--option [...]"); @@ -4810,7 +4742,7 @@ mod test { let mut o = Arg::new("opt") .long("option") .action(ArgAction::Set) - .min_values(1); + .number_of_values(1..); o._build(); assert_eq!(o.to_string(), "--option ..."); @@ -4821,7 +4753,7 @@ mod test { let mut o = Arg::new("opt") .short('o') .action(ArgAction::Set) - .min_values(0) + .number_of_values(0..) .value_names(&["file"]); o._build(); @@ -4833,7 +4765,7 @@ mod test { let mut o = Arg::new("opt") .short('o') .action(ArgAction::Set) - .min_values(1) + .number_of_values(1..) .value_names(&["file"]); o._build(); @@ -4921,7 +4853,7 @@ mod test { #[test] fn positional_display_zero_or_more_values() { - let mut p = Arg::new("pos").index(1).min_values(0); + let mut p = Arg::new("pos").index(1).number_of_values(0..); p._build(); assert_eq!(p.to_string(), "[...]"); @@ -4929,7 +4861,7 @@ mod test { #[test] fn positional_display_one_or_more_values() { - let mut p = Arg::new("pos").index(1).min_values(1); + let mut p = Arg::new("pos").index(1).number_of_values(1..); p._build(); assert_eq!(p.to_string(), "..."); diff --git a/src/error/kind.rs b/src/error/kind.rs index e25e01b9b71..a7853b34db8 100644 --- a/src/error/kind.rs +++ b/src/error/kind.rs @@ -113,7 +113,7 @@ pub enum ErrorKind { TooManyValues, /// Occurs when the user provides fewer values for an argument than were defined by setting - /// [`Arg::min_values`]. + /// [`Arg::number_of_values`]. /// /// # Examples /// @@ -122,12 +122,12 @@ pub enum ErrorKind { /// let result = Command::new("prog") /// .arg(Arg::new("some_opt") /// .long("opt") - /// .min_values(3)) + /// .number_of_values(3..)) /// .try_get_matches_from(vec!["prog", "--opt", "too", "few"]); /// assert!(result.is_err()); /// assert_eq!(result.unwrap_err().kind(), ErrorKind::TooFewValues); /// ``` - /// [`Arg::min_values`]: crate::Arg::min_values() + /// [`Arg::number_of_values`]: crate::Arg::number_of_values() TooFewValues, /// Occurs when the user provides a different number of values for an argument than what's diff --git a/src/output/help.rs b/src/output/help.rs index fff6c78499a..f3228831e97 100644 --- a/src/output/help.rs +++ b/src/output/help.rs @@ -296,8 +296,7 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> { debug!("Help::val: arg={}", arg.name); let mut need_closing_bracket = false; if arg.is_takes_value_set() && !arg.is_positional() { - let is_optional_val = arg.get_min_vals() == Some(0) - || arg.get_num_vals().map(|r| r.min_values()) == Some(0); + let is_optional_val = arg.get_min_vals() == Some(0); let sep = if arg.is_require_equals_set() { if is_optional_val { need_closing_bracket = true; diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index d8812b63773..f859afe69b0 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -225,9 +225,6 @@ impl ArgMatcher { } else if let Some(num) = o.max_vals { debug!("ArgMatcher::needs_more_vals: max_vals...{}", num); current_num < num - } else if o.get_min_vals().is_some() { - debug!("ArgMatcher::needs_more_vals: min_vals...true"); - true } else { o.is_multiple_values_set() } diff --git a/src/parser/matches/arg_matches.rs b/src/parser/matches/arg_matches.rs index 978c745ce4c..0c8dcb0da49 100644 --- a/src/parser/matches/arg_matches.rs +++ b/src/parser/matches/arg_matches.rs @@ -360,7 +360,7 @@ impl ArgMatches { /// let m = Command::new("myprog") /// .arg(Arg::new("exec") /// .short('x') - /// .min_values(1) + /// .number_of_values(1..) /// .action(ArgAction::Append) /// .value_terminator(";")) /// .get_matches_from(vec![ diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 69ad68b479a..0af36ba15fc 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -33,9 +33,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { let o = &self.cmd[&a]; let should_err = if let Some(v) = matcher.args.get(&o.id) { - v.all_val_groups_empty() - && (o.get_min_vals() != Some(0) - && o.get_num_vals().map(|r| r.min_values()) != Some(0)) + v.all_val_groups_empty() && o.get_min_vals() != Some(0) } else { true }; @@ -250,16 +248,29 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { fn validate_arg_num_vals(&self, a: &Arg, ma: &MatchedArg) -> ClapResult<()> { debug!("Validator::validate_arg_num_vals"); - let mut min_vals_zero = false; - - if let Some(expected) = a.num_vals { - for (_i, group) in ma.vals().enumerate() { - let actual = group.len(); - debug!( - "Validator::validate_arg_num_vals: group={}, num_vals={}, actual={}", - _i, expected, actual - ); - min_vals_zero |= expected.min_values() == 0; + for (_i, group) in ma.vals().enumerate() { + let actual = group.len(); + debug!( + "Validator::validate_arg_num_vals: group={}, actual={}", + _i, actual + ); + + let min_vals = a.get_min_vals().unwrap_or(1); + if a.is_takes_value_set() && 0 < min_vals && actual == 0 { + // Issue 665 (https://github.com/clap-rs/clap/issues/665) + // Issue 1105 (https://github.com/clap-rs/clap/issues/1105) + return Err(Error::empty_value( + self.cmd, + &get_possible_values(a) + .iter() + .filter(|pv| !pv.is_hide_set()) + .map(PossibleValue::get_name) + .collect::>(), + a.to_string(), + )); + } + + if let Some(expected) = a.num_vals { if let Some(expected) = expected.num_values() { if expected != actual { debug!( @@ -322,35 +333,6 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { )); } } - if let Some(num) = a.get_min_vals() { - debug!("Validator::validate_arg_num_vals: min_vals set: {}", num); - if ma.num_vals() < num && num != 0 { - debug!("Validator::validate_arg_num_vals: Sending error TooFewValues"); - return Err(Error::too_few_values( - self.cmd, - a.to_string(), - num, - ma.num_vals(), - Usage::new(self.cmd) - .required(&self.required) - .create_usage_with_title(&[]), - )); - } - min_vals_zero |= num == 0; - }; - // Issue 665 (https://github.com/clap-rs/clap/issues/665) - // Issue 1105 (https://github.com/clap-rs/clap/issues/1105) - if a.is_takes_value_set() && !min_vals_zero && ma.all_val_groups_empty() { - return Err(Error::empty_value( - self.cmd, - &get_possible_values(a) - .iter() - .filter(|pv| !pv.is_hide_set()) - .map(PossibleValue::get_name) - .collect::>(), - a.to_string(), - )); - } Ok(()) } diff --git a/tests/builder/default_missing_vals.rs b/tests/builder/default_missing_vals.rs index 6f747c84d5b..f00fb007ecb 100644 --- a/tests/builder/default_missing_vals.rs +++ b/tests/builder/default_missing_vals.rs @@ -7,7 +7,7 @@ fn opt_missing() { Arg::new("color") .long("color") .default_value("auto") - .min_values(0) + .number_of_values(0..=1) .require_equals(true) .default_missing_value("always"), ) @@ -33,7 +33,7 @@ fn opt_present_with_missing_value() { Arg::new("color") .long("color") .default_value("auto") - .min_values(0) + .number_of_values(0..=1) .require_equals(true) .default_missing_value("always"), ) @@ -59,7 +59,7 @@ fn opt_present_with_value() { Arg::new("color") .long("color") .default_value("auto") - .min_values(0) + .number_of_values(0..=1) .require_equals(true) .default_missing_value("always"), ) @@ -233,7 +233,7 @@ fn delimited_missing_value() { .long("flag") .default_value("one,two") .default_missing_value("three,four") - .min_values(0) + .number_of_values(0..) .value_delimiter(',') .require_equals(true), ); @@ -294,7 +294,7 @@ fn valid_index() { Arg::new("color") .long("color") .default_value("auto") - .min_values(0) + .number_of_values(0..=1) .require_equals(true) .default_missing_value("always"), ) diff --git a/tests/builder/help.rs b/tests/builder/help.rs index af61d56e0aa..188969e8c80 100644 --- a/tests/builder/help.rs +++ b/tests/builder/help.rs @@ -218,19 +218,19 @@ ARGS: ... tests specific values [possible values: vi, emacs] OPTIONS: - -o, --option ... tests options - -f, --flag tests flags - -F tests flags with exclusions - --long-option-2 tests long options with exclusions - -O, --option3 specific vals [possible values: fast, slow] - --multvals Tests multiple values, not mult occs - --multvalsmo Tests multiple values, and mult occs - --minvals2 ... Tests 2 min vals - --maxvals3 ... Tests 3 max vals - --optvaleq[=...] Tests optional value, require = sign - --optvalnoeq [...] Tests optional value - -h, --help Print help information - -V, --version Print version information + -o, --option ... tests options + -f, --flag tests flags + -F tests flags with exclusions + --long-option-2 tests long options with exclusions + -O, --option3 specific vals [possible values: fast, slow] + --multvals Tests multiple values, not mult occs + --multvalsmo Tests multiple values, and mult occs + --minvals2 ... Tests 2 min vals + --maxvals3 ... Tests 3 max vals + --optvaleq[=...] Tests optional value, require = sign + --optvalnoeq [...] Tests optional value + -h, --help Print help information + -V, --version Print version information SUBCOMMANDS: subcmd tests subcommands diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index e2df2f363de..92ba887e33a 100644 --- a/tests/builder/multiple_values.rs +++ b/tests/builder/multiple_values.rs @@ -202,10 +202,10 @@ fn option_min_exact() { Arg::new("option") .short('o') .help("multiple options") - .min_values(3) - .action(ArgAction::Append), + .number_of_values(3..) + .action(ArgAction::Set), ) - .try_get_matches_from(vec!["", "-o", "val1", "-o", "val2", "-o", "val3"]); + .try_get_matches_from(vec!["", "-o", "val1", "val2", "val3"]); assert!(m.is_ok(), "{}", m.unwrap_err()); let m = m.unwrap(); @@ -227,10 +227,10 @@ fn option_min_less() { Arg::new("option") .short('o') .help("multiple options") - .min_values(3) - .action(ArgAction::Append), + .number_of_values(3..) + .action(ArgAction::Set), ) - .try_get_matches_from(vec!["", "-o", "val1", "-o", "val2"]); + .try_get_matches_from(vec!["", "-o", "val1", "val2"]); assert!(m.is_err()); assert_eq!(m.unwrap_err().kind(), ErrorKind::TooFewValues); @@ -244,12 +244,10 @@ fn option_short_min_more_mult_occurs() { Arg::new("option") .short('o') .help("multiple options") - .min_values(3) - .action(ArgAction::Append), + .number_of_values(3..) + .action(ArgAction::Set), ) - .try_get_matches_from(vec![ - "", "pos", "-o", "val1", "-o", "val2", "-o", "val3", "-o", "val4", - ]); + .try_get_matches_from(vec!["", "pos", "-o", "val1", "val2", "val3", "val4"]); assert!(m.is_ok(), "{}", m.unwrap_err()); let m = m.unwrap(); @@ -274,7 +272,7 @@ fn option_short_min_more_single_occur() { Arg::new("option") .short('o') .help("multiple options") - .min_values(3), + .number_of_values(3..), ) .try_get_matches_from(vec!["", "pos", "-o", "val1", "val2", "val3", "val4"]); @@ -522,7 +520,11 @@ fn positional_exact_more() { #[test] fn positional_min_exact() { let m = Command::new("multiple_values") - .arg(Arg::new("pos").help("multiple positionals").min_values(3)) + .arg( + Arg::new("pos") + .help("multiple positionals") + .number_of_values(3..), + ) .try_get_matches_from(vec!["myprog", "val1", "val2", "val3"]); assert!(m.is_ok(), "{}", m.unwrap_err()); @@ -541,7 +543,11 @@ fn positional_min_exact() { #[test] fn positional_min_less() { let m = Command::new("multiple_values") - .arg(Arg::new("pos").help("multiple positionals").min_values(3)) + .arg( + Arg::new("pos") + .help("multiple positionals") + .number_of_values(3..), + ) .try_get_matches_from(vec!["myprog", "val1", "val2"]); assert!(m.is_err()); @@ -551,7 +557,11 @@ fn positional_min_less() { #[test] fn positional_min_more() { let m = Command::new("multiple_values") - .arg(Arg::new("pos").help("multiple positionals").min_values(3)) + .arg( + Arg::new("pos") + .help("multiple positionals") + .number_of_values(3..), + ) .try_get_matches_from(vec!["myprog", "val1", "val2", "val3", "val4"]); assert!(m.is_ok(), "{}", m.unwrap_err()); diff --git a/tests/builder/opts.rs b/tests/builder/opts.rs index 1d9b8900c05..0c6ba9f0708 100644 --- a/tests/builder/opts.rs +++ b/tests/builder/opts.rs @@ -71,7 +71,7 @@ fn require_equals_min_values_zero() { Arg::new("cfg") .action(ArgAction::Set) .require_equals(true) - .min_values(0) + .number_of_values(0..) .long("config"), ) .arg(Arg::new("cmd")) @@ -514,7 +514,7 @@ fn issue_1047_min_zero_vals_default_val() { .long("del") .action(ArgAction::Set) .require_equals(true) - .min_values(0) + .number_of_values(0..) .default_missing_value("default"), ) .try_get_matches_from(vec!["foo", "-d"]) diff --git a/tests/builder/require.rs b/tests/builder/require.rs index 55a477b8a67..c97d3ae6fd6 100644 --- a/tests/builder/require.rs +++ b/tests/builder/require.rs @@ -344,7 +344,7 @@ fn required_unless_present_err() { #[test] fn required_unless_present_with_optional_value() { let res = Command::new("unlesstest") - .arg(Arg::new("opt").long("opt").min_values(0).max_values(1)) + .arg(Arg::new("opt").long("opt").number_of_values(0..=1)) .arg( Arg::new("cfg") .required_unless_present("dbg") @@ -1144,7 +1144,7 @@ fn short_flag_require_equals_with_minvals_zero() { .arg( Arg::new("check") .short('c') - .min_values(0) + .number_of_values(0..) .require_equals(true), ) .arg(Arg::new("unique").short('u').action(ArgAction::SetTrue)) @@ -1162,7 +1162,7 @@ fn issue_2624() { .short('c') .long("check") .require_equals(true) - .min_values(0) + .number_of_values(0..) .value_parser(["silent", "quiet", "diagnose-first"]), ) .arg( diff --git a/tests/builder/utils.rs b/tests/builder/utils.rs index 3ee91acb53d..b458269656e 100644 --- a/tests/builder/utils.rs +++ b/tests/builder/utils.rs @@ -80,19 +80,17 @@ pub fn complex_app() -> Command<'static> { .required(false), arg!(--minvals2 "Tests 2 min vals") .required(false) - .min_values(2), + .number_of_values(2..), arg!(--maxvals3 "Tests 3 max vals") .required(false) .max_values(3), arg!(--optvaleq "Tests optional value, require = sign") .required(false) - .min_values(0) - .max_values(1) + .number_of_values(0..=1) .require_equals(true), arg!(--optvalnoeq "Tests optional value") .required(false) - .min_values(0) - .max_values(1), + .number_of_values(0..=1), ]) .subcommand( Command::new("subcmd") diff --git a/tests/derive/options.rs b/tests/derive/options.rs index eb2b433be96..7e2dd4ef1fb 100644 --- a/tests/derive/options.rs +++ b/tests/derive/options.rs @@ -428,7 +428,7 @@ fn option_vec_type() { fn option_vec_type_structopt_behavior() { #[derive(Parser, PartialEq, Debug)] struct Opt { - #[clap(short, long, multiple_values(true), min_values(0))] + #[clap(short, long, multiple_values(true), number_of_values(0..))] arg: Option>, } assert_eq!( From 30f5b11d063e1ce1a4b76aac84cd9259efb67c6d Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 28 Jul 2022 21:40:40 -0500 Subject: [PATCH 10/11] fix!: Replaced `min_values` (tota) with `number_of_values` (per occurrence) --- CHANGELOG.md | 3 +- clap_bench/benches/03_complex.rs | 4 +- src/builder/arg.rs | 66 ++------------------------------ src/error/kind.rs | 10 +++-- src/parser/arg_matcher.rs | 3 -- src/parser/validator.rs | 19 --------- tests/builder/multiple_values.rs | 55 +++++++++++++++----------- tests/builder/utils.rs | 2 +- 8 files changed, 47 insertions(+), 115 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b199bf6d1e9..8257bd888f0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,8 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `Arg::number_of_values` now applies per occurrence rather than the average across all occurrences - `number_of_values(0)` no longer implies `takes_value(true).multiple_values(true)` - `number_of_values(1)` no longer implies `multiple_values(true)` -- Remove `Arg::min_values` (across all occurrences) with `Arg::number_of_values` (per occurrence) +- Remove `Arg::min_values` (across all occurrences) with `Arg::number_of_values(N..)` (per occurrence) +- Remove `Arg::max_values` (across all occurrences) with `Arg::number_of_values(1..=M)` (per occurrence) - `ArgAction::SetTrue` and `ArgAction::SetFalse` now prioritize `Arg::default_missing_value` over their standard behavior - *(help)* Make `DeriveDisplayOrder` the default and removed the setting. To sort help, set `next_display_order(None)` (#2808) - *(help)* Subcommand display order respects `Command::next_display_order` instead of `DeriveDisplayOrder` and using its own initial display order value (#2808) diff --git a/clap_bench/benches/03_complex.rs b/clap_bench/benches/03_complex.rs index c56d7372954..c22c279e957 100644 --- a/clap_bench/benches/03_complex.rs +++ b/clap_bench/benches/03_complex.rs @@ -32,7 +32,7 @@ macro_rules! create_app { --multvalsmo "Tests multiple values, not mult occs" ).multiple_values(true).required(false).value_names(&["one", "two"]), arg!(--minvals2 ... "Tests 2 min vals").number_of_values(2..).multiple_values(true).required(false), - arg!(--maxvals3 ... "Tests 3 max vals").max_values(3).multiple_values(true).required(false), + arg!(--maxvals3 ... "Tests 3 max vals").number_of_values(1..=3).multiple_values(true).required(false), ]) .subcommand( Command::new("subcmd") @@ -132,7 +132,7 @@ pub fn build_from_builder(c: &mut Criterion) { .multiple_values(true) .action(ArgAction::Append) .help("Tests 3 max vals") - .max_values(3), + .number_of_values(1..=3), ) .subcommand( Command::new("subcmd") diff --git a/src/builder/arg.rs b/src/builder/arg.rs index cdc03ad59b6..61801eccb49 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -74,7 +74,6 @@ pub struct Arg<'help> { pub(crate) disp_ord: Option, pub(crate) val_names: Vec<&'help str>, pub(crate) num_vals: Option, - pub(crate) max_vals: Option, pub(crate) val_delim: Option, pub(crate) default_vals: Vec<&'help OsStr>, pub(crate) default_vals_ifs: Vec<(Id, ArgPredicate<'help>, Option<&'help OsStr>)>, @@ -1066,9 +1065,9 @@ impl<'help> Arg<'help> { /// /// [`subcommands`]: crate::Command::subcommand() /// [`Arg::number_of_values(1)`]: Arg::number_of_values() - /// [maximum number of values]: Arg::max_values() + /// [maximum number of values]: Arg::number_of_values() /// [specific number of values]: Arg::number_of_values() - /// [maximum]: Arg::max_values() + /// [maximum]: Arg::number_of_values() /// [specific]: Arg::number_of_values() #[inline] #[must_use] @@ -1236,63 +1235,6 @@ impl<'help> Arg<'help> { .multiple_values(qty.is_multiple()) } - /// The *maximum* number of values are for this argument. - /// - /// For example, if you had a - /// `-f ` argument where you wanted up to 3 'files' you would set `.max_values(3)`, and - /// this argument would be satisfied if the user provided, 1, 2, or 3 values. - /// - /// # Examples - /// - /// ```rust - /// # use clap::{Command, Arg}; - /// Arg::new("file") - /// .short('f') - /// .max_values(3); - /// ``` - /// - /// Supplying less than the maximum number of values is allowed - /// - /// ```rust - /// # use clap::{Command, Arg, ArgAction}; - /// let res = Command::new("prog") - /// .arg(Arg::new("file") - /// .action(ArgAction::Set) - /// .max_values(3) - /// .short('F')) - /// .try_get_matches_from(vec![ - /// "prog", "-F", "file1", "file2" - /// ]); - /// - /// assert!(res.is_ok()); - /// let m = res.unwrap(); - /// let files: Vec<_> = m.get_many::("file").unwrap().collect(); - /// assert_eq!(files, ["file1", "file2"]); - /// ``` - /// - /// Supplying more than the maximum number of values is an error - /// - /// ```rust - /// # use clap::{Command, Arg, ErrorKind, ArgAction}; - /// let res = Command::new("prog") - /// .arg(Arg::new("file") - /// .action(ArgAction::Set) - /// .max_values(2) - /// .short('F')) - /// .try_get_matches_from(vec![ - /// "prog", "-F", "file1", "file2", "file3" - /// ]); - /// - /// assert!(res.is_err()); - /// assert_eq!(res.unwrap_err().kind(), ErrorKind::UnknownArgument); - /// ``` - #[inline] - #[must_use] - pub fn max_values(mut self, qty: usize) -> Self { - self.max_vals = Some(qty); - self.takes_value(true).multiple_values(true) - } - /// Placeholder for the argument's value in the help message / usage. /// /// This name is cosmetic only; the name is **not** used to access arguments. @@ -1791,7 +1733,7 @@ impl<'help> Arg<'help> { /// By default when /// one sets [`multiple_values(true)`] on an argument, clap will continue parsing values for that /// argument until it reaches another valid argument, or one of the other more specific settings - /// for multiple values is used (such as [`max_values`] or [`number_of_values`]). + /// for multiple values is used (such as [`number_of_values`]). /// /// **NOTE:** This setting only applies to [options] and [positional arguments] /// @@ -1832,7 +1774,6 @@ impl<'help> Arg<'help> { /// [positional arguments]: Arg::index() /// [`multiple_values(true)`]: Arg::multiple_values() /// [`number_of_values`]: Arg::number_of_values() - /// [`max_values`]: Arg::max_values() #[inline] #[must_use] pub fn value_terminator(mut self, term: &'help str) -> Self { @@ -4543,7 +4484,6 @@ impl<'help> fmt::Debug for Arg<'help> { .field("disp_ord", &self.disp_ord) .field("val_names", &self.val_names) .field("num_vals", &self.num_vals) - .field("max_vals", &self.max_vals) .field("val_delim", &self.val_delim) .field("default_vals", &self.default_vals) .field("default_vals_ifs", &self.default_vals_ifs) diff --git a/src/error/kind.rs b/src/error/kind.rs index a7853b34db8..d0ce745cedb 100644 --- a/src/error/kind.rs +++ b/src/error/kind.rs @@ -96,7 +96,7 @@ pub enum ErrorKind { ValueValidation, /// Occurs when a user provides more values for an argument than were defined by setting - /// [`Arg::max_values`]. + /// [`Arg::number_of_values`]. /// /// # Examples /// @@ -104,12 +104,14 @@ pub enum ErrorKind { /// # use clap::{Command, Arg, ErrorKind}; /// let result = Command::new("prog") /// .arg(Arg::new("arg") - /// .max_values(2)) - /// .try_get_matches_from(vec!["prog", "too", "many", "values"]); + /// .number_of_values(1..=2) + /// .use_value_delimiter(true) + /// .require_value_delimiter(true)) + /// .try_get_matches_from(vec!["prog", "too,many,values"]); /// assert!(result.is_err()); /// assert_eq!(result.unwrap_err().kind(), ErrorKind::TooManyValues); /// ``` - /// [`Arg::max_values`]: crate::Arg::max_values() + /// [`Arg::number_of_values`]: crate::Arg::number_of_values() TooManyValues, /// Occurs when the user provides fewer values for an argument than were defined by setting diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs index f859afe69b0..5856d6e0ac6 100644 --- a/src/parser/arg_matcher.rs +++ b/src/parser/arg_matcher.rs @@ -222,9 +222,6 @@ impl ArgMatcher { expected, num_pending ); expected.accepts_more(num_pending) - } else if let Some(num) = o.max_vals { - debug!("ArgMatcher::needs_more_vals: max_vals...{}", num); - current_num < num } else { o.is_multiple_values_set() } diff --git a/src/parser/validator.rs b/src/parser/validator.rs index 0af36ba15fc..4f64215c58a 100644 --- a/src/parser/validator.rs +++ b/src/parser/validator.rs @@ -314,25 +314,6 @@ impl<'help, 'cmd> Validator<'help, 'cmd> { } } } - if let Some(num) = a.max_vals { - debug!("Validator::validate_arg_num_vals: max_vals set...{}", num); - if ma.num_vals() > num { - debug!("Validator::validate_arg_num_vals: Sending error TooManyValues"); - return Err(Error::too_many_values( - self.cmd, - ma.raw_vals_flatten() - .last() - .expect(INTERNAL_ERROR_MSG) - .to_str() - .expect(INVALID_UTF8) - .to_string(), - a.to_string(), - Usage::new(self.cmd) - .required(&self.required) - .create_usage_with_title(&[]), - )); - } - } Ok(()) } diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs index 92ba887e33a..8efba4f9020 100644 --- a/tests/builder/multiple_values.rs +++ b/tests/builder/multiple_values.rs @@ -298,10 +298,10 @@ fn option_max_exact() { Arg::new("option") .short('o') .help("multiple options") - .max_values(3) - .action(ArgAction::Append), + .number_of_values(1..=3) + .action(ArgAction::Set), ) - .try_get_matches_from(vec!["", "-o", "val1", "-o", "val2", "-o", "val3"]); + .try_get_matches_from(vec!["", "-o", "val1", "val2", "val3"]); assert!(m.is_ok(), "{}", m.unwrap_err()); let m = m.unwrap(); @@ -323,10 +323,10 @@ fn option_max_less() { Arg::new("option") .short('o') .help("multiple options") - .max_values(3) - .action(ArgAction::Append), + .number_of_values(1..=3) + .action(ArgAction::Set), ) - .try_get_matches_from(vec!["", "-o", "val1", "-o", "val2"]); + .try_get_matches_from(vec!["", "-o", "val1", "val2"]); assert!(m.is_ok(), "{}", m.unwrap_err()); let m = m.unwrap(); @@ -348,8 +348,8 @@ fn option_max_zero() { Arg::new("option") .short('o') .help("multiple options") - .max_values(3) - .action(ArgAction::Append), + .number_of_values(1..=3) + .action(ArgAction::Set), ) .try_get_matches_from(vec!["", "-o"]); @@ -364,8 +364,8 @@ fn option_max_zero_eq() { Arg::new("option") .short('o') .help("multiple options") - .max_values(3) - .action(ArgAction::Append), + .number_of_values(1..=3) + .action(ArgAction::Set), ) .try_get_matches_from(vec!["", "-o="]); @@ -389,15 +389,14 @@ fn option_max_more() { Arg::new("option") .short('o') .help("multiple options") - .max_values(3) - .action(ArgAction::Append), + .number_of_values(1..=3) + .action(ArgAction::Set), ) - .try_get_matches_from(vec![ - "", "-o", "val1", "-o", "val2", "-o", "val3", "-o", "val4", - ]); + .try_get_matches_from(vec!["", "-o", "val1", "val2", "val3", "val4"]); assert!(m.is_err()); - assert_eq!(m.unwrap_err().kind(), ErrorKind::TooManyValues); + // Can end up being TooManyValues or UnknownArgument + assert_eq!(m.unwrap_err().kind(), ErrorKind::UnknownArgument); } #[test] @@ -580,7 +579,11 @@ fn positional_min_more() { #[test] fn positional_max_exact() { let m = Command::new("multiple_values") - .arg(Arg::new("pos").help("multiple positionals").max_values(3)) + .arg( + Arg::new("pos") + .help("multiple positionals") + .number_of_values(1..=3), + ) .try_get_matches_from(vec!["myprog", "val1", "val2", "val3"]); assert!(m.is_ok(), "{}", m.unwrap_err()); @@ -599,7 +602,11 @@ fn positional_max_exact() { #[test] fn positional_max_less() { let m = Command::new("multiple_values") - .arg(Arg::new("pos").help("multiple positionals").max_values(3)) + .arg( + Arg::new("pos") + .help("multiple positionals") + .number_of_values(1..=3), + ) .try_get_matches_from(vec!["myprog", "val1", "val2"]); assert!(m.is_ok(), "{}", m.unwrap_err()); @@ -618,7 +625,11 @@ fn positional_max_less() { #[test] fn positional_max_more() { let m = Command::new("multiple_values") - .arg(Arg::new("pos").help("multiple positionals").max_values(3)) + .arg( + Arg::new("pos") + .help("multiple positionals") + .number_of_values(1..=3), + ) .try_get_matches_from(vec!["myprog", "val1", "val2", "val3", "val4"]); assert!(m.is_err()); @@ -1454,7 +1465,7 @@ fn multiple_vals_with_hyphen() { #[test] fn issue_1480_max_values_consumes_extra_arg_1() { let res = Command::new("prog") - .arg(Arg::new("field").max_values(1).long("field")) + .arg(Arg::new("field").number_of_values(..=1).long("field")) .arg(Arg::new("positional").required(true).index(1)) .try_get_matches_from(vec!["prog", "--field", "1", "file"]); @@ -1464,7 +1475,7 @@ fn issue_1480_max_values_consumes_extra_arg_1() { #[test] fn issue_1480_max_values_consumes_extra_arg_2() { let res = Command::new("prog") - .arg(Arg::new("field").max_values(1).long("field")) + .arg(Arg::new("field").number_of_values(..=1).long("field")) .try_get_matches_from(vec!["prog", "--field", "1", "2"]); assert!(res.is_err()); @@ -1474,7 +1485,7 @@ fn issue_1480_max_values_consumes_extra_arg_2() { #[test] fn issue_1480_max_values_consumes_extra_arg_3() { let res = Command::new("prog") - .arg(Arg::new("field").max_values(1).long("field")) + .arg(Arg::new("field").number_of_values(..=1).long("field")) .try_get_matches_from(vec!["prog", "--field", "1", "2", "3"]); assert!(res.is_err()); diff --git a/tests/builder/utils.rs b/tests/builder/utils.rs index b458269656e..55ab1ebe774 100644 --- a/tests/builder/utils.rs +++ b/tests/builder/utils.rs @@ -83,7 +83,7 @@ pub fn complex_app() -> Command<'static> { .number_of_values(2..), arg!(--maxvals3 "Tests 3 max vals") .required(false) - .max_values(3), + .number_of_values(1..=3), arg!(--optvaleq "Tests optional value, require = sign") .required(false) .number_of_values(0..=1) From eee31e2e31295e10de138dfdb56d15fada9d5091 Mon Sep 17 00:00:00 2001 From: Ed Page Date: Thu, 28 Jul 2022 21:50:32 -0500 Subject: [PATCH 11/11] docs: Provide best practice by example --- src/builder/arg.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/builder/arg.rs b/src/builder/arg.rs index 61801eccb49..ff5a96f1388 100644 --- a/src/builder/arg.rs +++ b/src/builder/arg.rs @@ -1093,7 +1093,7 @@ impl<'help> Arg<'help> { /// /// **WARNING:** /// - /// Setting a variable number of values (e.g. `..10`) for an argument without + /// Setting a variable number of values (e.g. `1..=10`) for an argument without /// other details can be dangerous in some circumstances. Because multiple values are /// allowed, `--option val1 val2 val3` is perfectly valid. Be careful when designing a CLI /// where **positional arguments** or **subcommands** are *also* expected as `clap` will continue