Repository: clap-rs/clap
Pull request #3999 (2022-07-28 20:01:40)

fix: Misc clean up in prep for #2688
Splitting this out and getting it merged so I can more easily iterate on different approaches to resolving #2688.


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index eead80e8..5fcfea3c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -20,6 +20,8 @@ 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`
+- `number_of_values(0)` no longer implies `takes_value(true).multiple_values(true)`
+- `number_of_values(1)` no longer implies `multiple_values(true)`
 - *(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)
 - *(env)* Parse `--help` and `--version` like any `ArgAction::SetTrue` flag (#3776)
diff --git a/src/builder/arg.rs b/src/builder/arg.rs
index e9d13a22..83feb802 100644
--- a/src/builder/arg.rs
+++ b/src/builder/arg.rs
@@ -1118,7 +1118,7 @@ impl<'help> Arg<'help> {
     #[must_use]
     pub fn number_of_values(mut self, qty: usize) -> Self {
         self.num_vals = Some(qty);
-        self.takes_value(true).multiple_values(true)
+        self.takes_value(qty != 0).multiple_values(1 < qty)
     }
 
     /// The *maximum* number of values are for this argument.
@@ -4066,6 +4066,11 @@ impl<'help> Arg<'help> {
         self.num_vals
     }
 
+    #[inline]
+    pub(crate) fn get_min_vals(&self) -> Option<usize> {
+        self.min_vals
+    }
+
     /// Get the delimiter between multiple values
     #[inline]
     pub fn get_value_delimiter(&self) -> Option<char> {
@@ -4425,8 +4430,14 @@ impl<'help> Display for Arg<'help> {
             write!(f, "-{}", s)?;
         }
         let mut need_closing_bracket = false;
-        if !self.is_positional() && self.is_takes_value_set() {
-            let is_optional_val = self.min_vals == Some(0);
+        let is_optional_val = self.get_min_vals() == Some(0);
+        if self.is_positional() {
+            if is_optional_val {
+                let sep = "[";
+                need_closing_bracket = true;
+                f.write_str(sep)?;
+            }
+        } else if self.is_takes_value_set() {
             let sep = if self.is_require_equals_set() {
                 if is_optional_val {
                     need_closing_bracket = true;
@@ -4443,7 +4454,8 @@ impl<'help> Display for Arg<'help> {
             f.write_str(sep)?;
         }
         if self.is_takes_value_set() || self.is_positional() {
-            display_arg_val(self, |s, _| f.write_str(s))?;
+            let arg_val = render_arg_val(self);
+            f.write_str(&arg_val)?;
         }
         if need_closing_bracket {
             f.write_str("]")?;
@@ -4514,73 +4526,63 @@ impl Default for ArgProvider {
 }
 
 /// Write the values such as <name1> <name2>
-pub(crate) fn display_arg_val<F, T, E>(arg: &Arg, mut write: F) -> Result<(), E>
-where
-    F: FnMut(&str, bool) -> Result<T, E>,
-{
-    let mult_val = arg.is_multiple_values_set();
-    let mult_occ = matches!(*arg.get_action(), ArgAction::Append);
+pub(crate) fn render_arg_val(arg: &Arg) -> String {
+    let mut rendered = String::new();
+
+    let delim_storage;
     let delim = if arg.is_require_value_delimiter_set() {
-        arg.val_delim.expect(INTERNAL_ERROR_MSG)
+        delim_storage = arg.val_delim.expect(INTERNAL_ERROR_MSG).to_string();
+        &delim_storage
     } else {
-        ' '
-    }
-    .to_string();
-    if !arg.val_names.is_empty() {
-        // If have val_name.
-        match (arg.val_names.len(), arg.num_vals) {
-            (1, Some(num_vals)) => {
-                // If single value name with multiple num_of_vals, display all
-                // the values with the single value name.
-                let arg_name = format!("<{}>", arg.val_names.get(0).unwrap());
-                for n in 1..=num_vals {
-                    write(&arg_name, true)?;
-                    if n != num_vals {
-                        write(&delim, false)?;
-                    }
-                }
-            }
-            (num_val_names, _) => {
-                // If multiple value names, display them sequentially(ignore num of vals).
-                let mut it = arg.val_names.iter().peekable();
-                while let Some(val) = it.next() {
-                    write(&format!("<{}>", val), true)?;
-                    if it.peek().is_some() {
-                        write(&delim, false)?;
-                    }
-                }
-                if (num_val_names == 1 && mult_val)
-                    || (arg.is_positional() && mult_occ)
-                    || num_val_names < arg.num_vals.unwrap_or(0)
-                {
-                    write("...", true)?;
-                }
+        " "
+    };
+
+    let arg_name_storage;
+    let val_names = if arg.val_names.is_empty() {
+        arg_name_storage = [arg.name];
+        &arg_name_storage
+    } else {
+        arg.val_names.as_slice()
+    };
+
+    let mut extra_values = false;
+    debug_assert!(arg.is_takes_value_set());
+    let num_vals = arg.num_vals.unwrap_or(1);
+    if val_names.len() == 1 {
+        let arg_name = format!("<{}>", val_names[0]);
+        for n in 1..=num_vals {
+            if n != 1 {
+                rendered.push_str(delim);
             }
+            rendered.push_str(&arg_name);
         }
-    } else if let Some(num_vals) = arg.num_vals {
-        // If number_of_values is specified, display the value multiple times.
-        let arg_name = format!("<{}>", arg.name);
-        for n in 1..=num_vals {
-            write(&arg_name, true)?;
-            if n != num_vals {
-                write(&delim, false)?;
+        extra_values |= arg.num_vals.is_none() && arg.is_multiple_values_set();
+    } else {
+        debug_assert!(1 < val_names.len());
+        for (n, val_name) in val_names.iter().enumerate() {
+            let arg_name = format!("<{}>", val_name);
+            if n != 0 {
+                rendered.push_str(delim);
             }
+            rendered.push_str(&arg_name);
         }
-    } else if arg.is_positional() {
-        // Value of positional argument with no num_vals and val_names.
-        write(&format!("<{}>", arg.name), true)?;
-
-        if mult_val || mult_occ {
-            write("...", true)?;
+        extra_values |= val_names.len() < num_vals;
+    }
+    if arg.is_positional() {
+        if matches!(*arg.get_action(), ArgAction::Append) {
+            extra_values = true;
         }
     } else {
-        // value of flag argument with no num_vals and val_names.
-        write(&format!("<{}>", arg.name), true)?;
-        if mult_val {
-            write("...", true)?;
+        if matches!(*arg.get_action(), ArgAction::Count) {
+            extra_values = true;
         }
     }
-    Ok(())
+
+    if extra_values {
+        rendered.push_str("...");
+    }
+
+    rendered
 }
 
 // Flags
@@ -4667,7 +4669,53 @@ mod test {
     }
 
     #[test]
-    fn option_display2() {
+    fn option_display_zero_or_more_values() {
+        let mut o = Arg::new("opt")
+            .long("option")
+            .action(ArgAction::Set)
+            .min_values(0);
+        o._build();
+
+        assert_eq!(o.to_string(), "--option [<opt>...]");
+    }
+
+    #[test]
+    fn option_display_one_or_more_values() {
+        let mut o = Arg::new("opt")
+            .long("option")
+            .action(ArgAction::Set)
+            .min_values(1);
+        o._build();
+
+        assert_eq!(o.to_string(), "--option <opt>...");
+    }
+
+    #[test]
+    fn option_display_zero_or_more_values_with_value_name() {
+        let mut o = Arg::new("opt")
+            .short('o')
+            .action(ArgAction::Set)
+            .min_values(0)
+            .value_names(&["file"]);
+        o._build();
+
+        assert_eq!(o.to_string(), "-o [<file>...]");
+    }
+
+    #[test]
+    fn option_display_one_or_more_values_with_value_name() {
+        let mut o = Arg::new("opt")
+            .short('o')
+            .action(ArgAction::Set)
+            .min_values(1)
+            .value_names(&["file"]);
+        o._build();
+
+        assert_eq!(o.to_string(), "-o <file>...");
+    }
+
+    #[test]
+    fn option_display_value_names() {
         let mut o = Arg::new("opt")
             .short('o')
             .action(ArgAction::Set)
@@ -4745,6 +4793,22 @@ mod test {
         assert_eq!(p.to_string(), "<pos>...");
     }
 
+    #[test]
+    fn positional_display_zero_or_more_values() {
+        let mut p = Arg::new("pos").index(1).min_values(0);
+        p._build();
+
+        assert_eq!(p.to_string(), "[<pos>...]");
+    }
+
+    #[test]
+    fn positional_display_one_or_more_values() {
+        let mut p = Arg::new("pos").index(1).min_values(1);
+        p._build();
+
+        assert_eq!(p.to_string(), "<pos>...");
+    }
+
     #[test]
     fn positional_display_multiple_occurrences() {
         let mut p = Arg::new("pos").index(1).action(ArgAction::Append);
diff --git a/src/builder/debug_asserts.rs b/src/builder/debug_asserts.rs
index fc84e109..4bb3f26b 100644
--- a/src/builder/debug_asserts.rs
+++ b/src/builder/debug_asserts.rs
@@ -695,6 +695,13 @@ fn assert_arg(arg: &Arg) {
             arg.name, num_val_names, num_vals
         );
     }
+    if arg.get_num_vals() == Some(1) {
+        assert!(
+            !arg.is_multiple_values_set(),
+            "Argument {}: mismatch between `number_of_values` and `multiple_values`",
+            arg.name
+        );
+    }
 
     assert_arg_flags(arg);
 
diff --git a/src/builder/mod.rs b/src/builder/mod.rs
index 09b92e60..f66fb485 100644
--- a/src/builder/mod.rs
+++ b/src/builder/mod.rs
@@ -42,6 +42,6 @@ pub use value_parser::OsStringValueParser;
 pub use value_parser::PathBufValueParser;
 
 pub(crate) use action::CountType;
-pub(crate) use arg::display_arg_val;
+pub(crate) use arg::render_arg_val;
 pub(crate) use arg_predicate::ArgPredicate;
 pub(crate) use arg_settings::{ArgFlags, ArgSettings};
diff --git a/src/lib.rs b/src/lib.rs
index a27493e5..e8ae2015 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -91,6 +91,8 @@
 #![allow(clippy::branches_sharing_code)]
 // Doesn't allow for debug statements, etc to be unique
 #![allow(clippy::if_same_then_else)]
+// Breaks up parallelism that clarifies intent
+#![allow(clippy::collapsible_else_if)]
 
 #[cfg(not(feature = "std"))]
 compile_error!("`std` feature is currently required to build `clap`");
diff --git a/src/output/help.rs b/src/output/help.rs
index cb22dc69..f3228831 100644
--- a/src/output/help.rs
+++ b/src/output/help.rs
@@ -6,7 +6,7 @@ use std::io::{self, Write};
 use std::usize;
 
 // Internal
-use crate::builder::{display_arg_val, Arg, Command};
+use crate::builder::{render_arg_val, Arg, Command};
 use crate::output::{fmt::Colorizer, Usage};
 use crate::PossibleValue;
 
@@ -296,7 +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.min_vals == 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;
@@ -314,10 +314,8 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> {
         }
 
         if arg.is_takes_value_set() || arg.is_positional() {
-            display_arg_val(
-                arg,
-                |s, good| if good { self.good(s) } else { self.none(s) },
-            )?;
+            let arg_val = render_arg_val(arg);
+            self.good(arg_val)?;
         }
 
         if need_closing_bracket {
diff --git a/src/parser/arg_matcher.rs b/src/parser/arg_matcher.rs
index e09f6112..bf306b26 100644
--- a/src/parser/arg_matcher.rs
+++ b/src/parser/arg_matcher.rs
@@ -227,7 +227,7 @@ impl ArgMatcher {
         } else if let Some(num) = o.max_vals {
             debug!("ArgMatcher::needs_more_vals: max_vals...{}", num);
             current_num < num
-        } else if o.min_vals.is_some() {
+        } else if o.get_min_vals().is_some() {
             debug!("ArgMatcher::needs_more_vals: min_vals...true");
             true
         } else {
diff --git a/src/parser/parser.rs b/src/parser/parser.rs
index 79cf4445..a427497d 100644
--- a/src/parser/parser.rs
+++ b/src/parser/parser.rs
@@ -962,7 +962,7 @@ impl<'help, 'cmd> Parser<'help, 'cmd> {
         debug!("Parser::parse_opt_value; Checking for val...");
         // require_equals is set, but no '=' is provided, try throwing error.
         if arg.is_require_equals_set() && !has_eq {
-            if arg.min_vals == Some(0) {
+            if arg.get_min_vals() == Some(0) {
                 debug!("Requires equals, but min_vals == 0");
                 let arg_values = Vec::new();
                 let react_result = self.react(
diff --git a/src/parser/validator.rs b/src/parser/validator.rs
index 06c67c21..7e58cda7 100644
--- a/src/parser/validator.rs
+++ b/src/parser/validator.rs
@@ -34,7 +34,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.min_vals.is_some() && o.min_vals.unwrap() == 0)
+                v.all_val_groups_empty() && o.get_min_vals() != Some(0)
             } else {
                 true
             };
@@ -296,7 +296,7 @@ impl<'help, 'cmd> Validator<'help, 'cmd> {
                 ));
             }
         }
-        let min_vals_zero = if let Some(num) = a.min_vals {
+        let min_vals_zero = 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");


Test patch:
diff --git a/tests/builder/app_settings.rs b/tests/builder/app_settings.rs
index 496427d4..a415f8c6 100644
--- a/tests/builder/app_settings.rs
+++ b/tests/builder/app_settings.rs
@@ -1149,7 +1149,6 @@ fn aaos_opts_mult_req_delims() {
     let res = Command::new("posix")
         .arg(
             arg!(--opt <val> ... "some option")
-                .number_of_values(1)
                 .action(ArgAction::Set)
                 .use_value_delimiter(true)
                 .require_value_delimiter(true)
diff --git a/tests/builder/help.rs b/tests/builder/help.rs
index 3888a67e..64d08b0b 100644
--- a/tests/builder/help.rs
+++ b/tests/builder/help.rs
@@ -218,19 +218,19 @@ ARGS:
     <positional3>...    tests specific values [possible values: vi, emacs]
 
 OPTIONS:
-    -o, --option <opt>...            tests options
-    -f, --flag                       tests flags
-    -F                               tests flags with exclusions
-        --long-option-2 <option2>    tests long options with exclusions
-    -O, --option3 <option3>          specific vals [possible values: fast, slow]
-        --multvals <one> <two>       Tests multiple values, not mult occs
-        --multvalsmo <one> <two>     Tests multiple values, and mult occs
-        --minvals2 <minvals>...      Tests 2 min vals
-        --maxvals3 <maxvals>...      Tests 3 max vals
-        --optvaleq[=<optval>]        Tests optional value, require = sign
-        --optvalnoeq [<optval>]      Tests optional value
-    -h, --help                       Print help information
-    -V, --version                    Print version information
+    -o, --option <opt>...             tests options
+    -f, --flag                        tests flags
+    -F                                tests flags with exclusions
+        --long-option-2 <option2>     tests long options with exclusions
+    -O, --option3 <option3>           specific vals [possible values: fast, slow]
+        --multvals <one> <two>        Tests multiple values, not mult occs
+        --multvalsmo <one> <two>      Tests multiple values, and mult occs
+        --minvals2 <minvals>...       Tests 2 min vals
+        --maxvals3 <maxvals>...       Tests 3 max vals
+        --optvaleq[=<optval>...]      Tests optional value, require = sign
+        --optvalnoeq [<optval>...]    Tests optional value
+    -h, --help                        Print help information
+    -V, --version                     Print version information
 
 SUBCOMMANDS:
     subcmd    tests subcommands
@@ -1044,21 +1044,9 @@ OPTIONS:
 }
 
 #[test]
-fn issue_760() {
-    // Using number_of_values(1) with multiple_values(true) misaligns help message
-    static ISSUE_760: &str = "ctest 0.1
-
-USAGE:
-    ctest [OPTIONS]
-
-OPTIONS:
-    -o, --option <option>    tests options
-    -O, --opt <opt>          tests options
-    -h, --help               Print help information
-    -V, --version            Print version information
-";
-
-    let cmd = Command::new("ctest")
+#[should_panic = "Argument option: mismatch between `number_of_values` and `multiple_values`"]
+fn number_of_values_conflicts_with_multiple_values() {
+    Command::new("ctest")
         .version("0.1")
         .arg(
             Arg::new("option")
@@ -1066,43 +1054,10 @@ OPTIONS:
                 .short('o')
                 .long("option")
                 .action(ArgAction::Set)
-                .multiple_values(true)
-                .number_of_values(1),
+                .number_of_values(1)
+                .multiple_values(true),
         )
-        .arg(
-            Arg::new("opt")
-                .help("tests options")
-                .short('O')
-                .long("opt")
-                .action(ArgAction::Set),
-        );
-    utils::assert_output(cmd, "ctest --help", ISSUE_760, false);
-}
-
-#[test]
-fn issue_1571() {
-    let cmd = Command::new("hello").arg(
-        Arg::new("name")
-            .long("package")
-            .short('p')
-            .number_of_values(1)
-            .action(ArgAction::Set)
-            .multiple_values(true),
-    );
-    utils::assert_output(
-        cmd,
-        "hello --help",
-        "hello 
-
-USAGE:
-    hello [OPTIONS]
-
-OPTIONS:
-    -p, --package <name>    
-    -h, --help              Print help information
-",
-        false,
-    );
+        .build();
 }
 
 static RIPGREP_USAGE: &str = "ripgrep 0.5
diff --git a/tests/builder/multiple_values.rs b/tests/builder/multiple_values.rs
index 0cfcfadc..197f98ca 100644
--- a/tests/builder/multiple_values.rs
+++ b/tests/builder/multiple_values.rs
@@ -236,7 +236,7 @@ fn option_min_less() {
 
 #[test]
 fn option_short_min_more_mult_occurs() {
-    let res = Command::new("multiple_values")
+    let m = Command::new("multiple_values")
         .arg(Arg::new("arg").required(true))
         .arg(
             Arg::new("option")
@@ -249,8 +249,8 @@ fn option_short_min_more_mult_occurs() {
             "", "pos", "-o", "val1", "-o", "val2", "-o", "val3", "-o", "val4",
         ]);
 
-    assert!(res.is_ok(), "{:?}", res.unwrap_err().kind());
-    let m = res.unwrap();
+    assert!(m.is_ok(), "{}", m.unwrap_err());
+    let m = m.unwrap();
 
     assert!(m.contains_id("option"));
     assert!(m.contains_id("arg"));
@@ -266,7 +266,7 @@ fn option_short_min_more_mult_occurs() {
 
 #[test]
 fn option_short_min_more_single_occur() {
-    let res = Command::new("multiple_values")
+    let m = Command::new("multiple_values")
         .arg(Arg::new("arg").required(true))
         .arg(
             Arg::new("option")
@@ -276,8 +276,8 @@ fn option_short_min_more_single_occur() {
         )
         .try_get_matches_from(vec!["", "pos", "-o", "val1", "val2", "val3", "val4"]);
 
-    assert!(res.is_ok(), "{:?}", res.unwrap_err().kind());
-    let m = res.unwrap();
+    assert!(m.is_ok(), "{}", m.unwrap_err());
+    let m = m.unwrap();
 
     assert!(m.contains_id("option"));
     assert!(m.contains_id("arg"));
@@ -1386,22 +1386,6 @@ fn issue_1480_max_values_consumes_extra_arg_3() {
     assert_eq!(res.unwrap_err().kind(), ErrorKind::UnknownArgument);
 }
 
-#[test]
-fn issue_2229() {
-    let m = Command::new("multiple_values")
-        .arg(
-            Arg::new("pos")
-                .help("multiple positionals")
-                .number_of_values(3),
-        )
-        .try_get_matches_from(vec![
-            "myprog", "val1", "val2", "val3", "val4", "val5", "val6",
-        ]);
-
-    assert!(m.is_err());
-    assert_eq!(m.unwrap_err().kind(), ErrorKind::WrongNumberOfValues);
-}
-
 #[test]
 fn value_names_building_num_vals() {
     let m = Command::new("test")
@@ -1412,7 +1396,7 @@ fn value_names_building_num_vals() {
         )
         .try_get_matches_from(vec!["myprog", "--pos", "val1", "val2", "val3"]);
 
-    assert!(m.is_ok(), "{:?}", m.unwrap_err().kind());
+    assert!(m.is_ok(), "{}", m.unwrap_err());
     let m = m.unwrap();
 
     assert_eq!(
@@ -1430,7 +1414,7 @@ fn value_names_building_num_vals_for_positional() {
         .arg(Arg::new("pos").value_names(&["who", "what", "why"]))
         .try_get_matches_from(vec!["myprog", "val1", "val2", "val3"]);
 
-    assert!(m.is_ok(), "{:?}", m.unwrap_err().kind());
+    assert!(m.is_ok(), "{}", m.unwrap_err());
     let m = m.unwrap();
 
     assert_eq!(
@@ -1453,7 +1437,7 @@ fn number_of_values_preferred_over_value_names() {
         )
         .try_get_matches_from(vec!["myprog", "--pos", "val1", "val2", "val3", "val4"]);
 
-    assert!(m.is_ok(), "{:?}", m.unwrap_err().kind());
+    assert!(m.is_ok(), "{}", m.unwrap_err());
     let m = m.unwrap();
 
     assert_eq!(
@@ -1475,10 +1459,8 @@ fn values_per_occurrence_named() {
     );
 
     let m = a.try_get_matches_from_mut(vec!["myprog", "--pos", "val1", "val2"]);
-    let m = match m {
-        Ok(m) => m,
-        Err(err) => panic!("{}", err),
-    };
+    assert!(m.is_ok(), "{}", m.unwrap_err());
+    let m = m.unwrap();
     assert_eq!(
         m.get_many::<String>("pos")
             .unwrap()
@@ -1512,10 +1494,8 @@ fn values_per_occurrence_positional() {
     );
 
     let m = a.try_get_matches_from_mut(vec!["myprog", "val1", "val2"]);
-    let m = match m {
-        Ok(m) => m,
-        Err(err) => panic!("{}", err),
-    };
+    assert!(m.is_ok(), "{}", m.unwrap_err());
+    let m = m.unwrap();
     assert_eq!(
         m.get_many::<String>("pos")
             .unwrap()
@@ -1524,3 +1504,19 @@ fn values_per_occurrence_positional() {
         ["val1", "val2"]
     );
 }
+
+#[test]
+fn issue_2229() {
+    let m = Command::new("multiple_values")
+        .arg(
+            Arg::new("pos")
+                .help("multiple positionals")
+                .number_of_values(3),
+        )
+        .try_get_matches_from(vec![
+            "myprog", "val1", "val2", "val3", "val4", "val5", "val6",
+        ]);
+
+    assert!(m.is_err());
+    assert_eq!(m.unwrap_err().kind(), ErrorKind::WrongNumberOfValues);
+}
diff --git a/tests/builder/opts.rs b/tests/builder/opts.rs
index 5707063d..1d9b8900 100644
--- a/tests/builder/opts.rs
+++ b/tests/builder/opts.rs
@@ -481,7 +481,6 @@ fn leading_hyphen_with_only_pos_follows() {
     let r = Command::new("mvae")
         .arg(
             arg!(o: -o [opt] ... "some opt")
-                .number_of_values(1)
                 .action(ArgAction::Set)
                 .allow_hyphen_values(true),
         )
diff --git a/tests/builder/utils.rs b/tests/builder/utils.rs
index d586fa42..3ee91acb 100644
--- a/tests/builder/utils.rs
+++ b/tests/builder/utils.rs
@@ -87,12 +87,12 @@ pub fn complex_app() -> Command<'static> {
             arg!(--optvaleq <optval> "Tests optional value, require = sign")
                 .required(false)
                 .min_values(0)
-                .number_of_values(1)
+                .max_values(1)
                 .require_equals(true),
             arg!(--optvalnoeq <optval> "Tests optional value")
                 .required(false)
                 .min_values(0)
-                .number_of_values(1),
+                .max_values(1),
         ])
         .subcommand(
             Command::new("subcmd")
diff --git a/tests/macros.rs b/tests/macros.rs
index 84b620a8..f95153e4 100644
--- a/tests/macros.rs
+++ b/tests/macros.rs
@@ -197,6 +197,43 @@ mod arg {
         assert!(arg.is_required_set());
         assert_eq!(arg.get_help(), Some("How to use it"));
     }
+
+    #[test]
+    fn optional_value() {
+        let mut cmd = clap::Command::new("test").arg(clap::arg!(port: -p [NUM]));
+
+        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::<String>("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::<String>("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::<String>("port").unwrap(), "42");
+
+        let mut help = Vec::new();
+        cmd.write_help(&mut help).unwrap();
+        const HELP: &str = "\
+test 
+
+USAGE:
+    test [OPTIONS]
+
+OPTIONS:
+    -p [<NUM>...]        
+    -h, --help           Print help information
+";
+        snapbox::assert_eq(HELP, help);
+    }
 }
 
 mod arg_impl {