Fix handling of number_of_values when displaying arg (clap-rs#2701)

* Remove dead logic * Outline common logic * Fix issue 1571
epage · Aug 17, 2021 · 0394ae6 · 0394ae6
1 parent b1d364a
commit 0394ae6
Show file tree

Hide file tree

Showing 3 changed files with 115 additions and 152 deletions.
diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs
@@ -15,7 +15,7 @@ use std::{
     error::Error,
     ffi::OsStr,
     fmt::{self, Display, Formatter},
-    str,
+    iter, str,
     sync::{Arc, Mutex},
 };
 #[cfg(feature = "env")]
@@ -4847,104 +4847,92 @@ impl<'help> PartialEq for Arg<'help> {
     }
 }
 
-impl<'help> Display for Arg<'help> {
-    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
-        if self.index.is_some() || self.is_positional() {
-            // Positional
-            let mut delim = String::new();
-
-            delim.push(if self.is_set(ArgSettings::RequireDelimiter) {
-                self.val_delim.expect(INTERNAL_ERROR_MSG)
-            } else {
-                ' '
-            });
-
-            if !self.val_names.is_empty() {
-                write!(
-                    f,
-                    "{}",
-                    self.val_names
-                        .iter()
-                        .map(|n| format!("<{}>", n))
-                        .collect::<Vec<_>>()
-                        .join(&*delim)
-                )?;
-            } else {
-                write!(f, "<{}>", self.name)?;
+/// 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_set(ArgSettings::MultipleValues);
+    let mult_occ = arg.is_set(ArgSettings::MultipleOccurrences);
+    let delim = if arg.is_set(ArgSettings::RequireDelimiter) {
+        arg.val_delim.expect(INTERNAL_ERROR_MSG)
+    } else {
+        ' '
+    };
+    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());
+                let mut it = iter::repeat(arg_name).take(num_vals).peekable();
+                while let Some(arg_name) = it.next() {
+                    write(&arg_name, true)?;
+                    if it.peek().is_some() {
+                        write(&delim.to_string(), false)?;
+                    }
+                }
             }
-
-            write!(f, "{}", self.multiple_str())?;
-
-            return Ok(());
-        } else if !self.is_set(ArgSettings::TakesValue) {
-            // Flag
-            if let Some(l) = self.long {
-                write!(f, "--{}", l)?;
-            } else if let Some(s) = self.short {
-                write!(f, "-{}", s)?;
+            (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.to_string(), false)?;
+                    }
+                }
+                if num_val_names == 1 && mult_val {
+                    write("...", true)?;
+                }
+            }
+        }
+    } else if let Some(num_vals) = arg.num_vals {
+        // If number_of_values is sepcified, display the value multiple times.
+        let arg_name = format!("<{}>", arg.name);
+        let mut it = iter::repeat(&arg_name).take(num_vals).peekable();
+        while let Some(arg_name) = it.next() {
+            write(arg_name, true)?;
+            if it.peek().is_some() {
+                write(&delim.to_string(), false)?;
             }
-
-            return Ok(());
         }
+    } else if arg.is_positional() {
+        // Value of positional argument with no num_vals and val_names.
+        write(&format!("<{}>", arg.name), true)?;
 
-        let sep = if self.is_set(ArgSettings::RequireEquals) {
-            "="
-        } else {
-            " "
-        };
+        if mult_val || mult_occ {
+            write("...", true)?;
+        }
+    } else {
+        // value of flag argument with no num_vals and val_names.
+        write(&format!("<{}>", arg.name), true)?;
+        if mult_val {
+            write("...", true)?;
+        }
+    }
+    Ok(())
+}
 
+impl<'help> Display for Arg<'help> {
+    fn fmt(&self, f: &mut Formatter) -> fmt::Result {
         // Write the name such --long or -l
         if let Some(l) = self.long {
-            write!(f, "--{}{}", l, sep)?;
-        } else {
-            write!(f, "-{}{}", self.short.unwrap(), sep)?;
+            write!(f, "--{}", l)?;
+        } else if let Some(s) = self.short {
+            write!(f, "-{}", s)?;
         }
-
-        let delim = if self.is_set(ArgSettings::RequireDelimiter) {
-            self.val_delim.expect(INTERNAL_ERROR_MSG)
-        } else {
-            ' '
-        };
-
-        // Write the values such as <name1> <name2>
-        if !self.val_names.is_empty() {
-            let num = self.val_names.len();
-            let mut it = self.val_names.iter().peekable();
-
-            while let Some(val) = it.next() {
-                write!(f, "<{}>", val)?;
-                if it.peek().is_some() {
-                    write!(f, "{}", delim)?;
-                }
-            }
-
-            if self.is_set(ArgSettings::MultipleValues) && num == 1 {
-                write!(f, "...")?;
-            }
-        } else if let Some(num) = self.num_vals {
-            let mut it = (0..num).peekable();
-
-            while let Some(_) = it.next() {
-                write!(f, "<{}>", self.name)?;
-                if it.peek().is_some() {
-                    write!(f, "{}", delim)?;
-                }
-            }
-
-            if self.is_set(ArgSettings::MultipleValues) && num == 1 {
-                write!(f, "...")?;
-            }
-        } else {
-            write!(
-                f,
-                "<{}>{}",
-                self.name,
-                if self.is_set(ArgSettings::MultipleValues) {
-                    "..."
-                } else {
-                    ""
-                }
-            )?;
+        if !self.is_positional() && self.is_set(ArgSettings::TakesValue) {
+            let sep = if self.is_set(ArgSettings::RequireEquals) {
+                "="
+            } else {
+                " "
+            };
+            write!(f, "{}", sep)?;
+        }
+        if self.is_set(ArgSettings::TakesValue) || self.is_positional() {
+            display_arg_val(self, |s, _| write!(f, "{}", s))?;
         }
 
         Ok(())

diff --git a/src/output/help.rs b/src/output/help.rs
@@ -9,11 +9,10 @@ use std::{
 
 // Internal
 use crate::{
-    build::{App, AppSettings, Arg, ArgSettings},
+    build::{arg::display_arg_val, App, AppSettings, Arg, ArgSettings},
     output::{fmt::Colorizer, Usage},
     parse::Parser,
     util::VecMap,
-    INTERNAL_ERROR_MSG,
 };
 
 // Third party
@@ -323,63 +322,11 @@ impl<'help, 'app, 'parser, 'writer> Help<'help, 'app, 'parser, 'writer> {
     /// Writes argument's possible values to the wrapped stream.
     fn val(&mut self, arg: &Arg<'help>, next_line_help: bool, longest: usize) -> io::Result<()> {
         debug!("Help::val: arg={}", arg.name);
-        let mult =
-            arg.is_set(ArgSettings::MultipleValues) || arg.is_set(ArgSettings::MultipleOccurrences);
-        if arg.is_set(ArgSettings::TakesValue) || arg.index.is_some() {
-            let delim = if arg.is_set(ArgSettings::RequireDelimiter) {
-                arg.val_delim.expect(INTERNAL_ERROR_MSG)
-            } else {
-                ' '
-            };
-            if !arg.val_names.is_empty() {
-                match (arg.val_names.len(), arg.num_vals) {
-                    (1, Some(num)) if num > 1 => {
-                        let arg_name = format!("<{}>", arg.val_names.get(0).unwrap());
-                        let mut it = (0..num).peekable();
-                        while it.next().is_some() {
-                            self.good(&arg_name)?;
-                            if it.peek().is_some() {
-                                self.none(&delim.to_string())?;
-                            }
-                        }
-                        if mult && num == 1 {
-                            self.good("...")?;
-                        }
-                    }
-                    _ => {
-                        let mut it = arg.val_names.iter().peekable();
-                        while let Some(val) = it.next() {
-                            self.good(&format!("<{}>", val))?;
-                            if it.peek().is_some() {
-                                self.none(&delim.to_string())?;
-                            }
-                        }
-                        let num = arg.val_names.len();
-                        if mult && num == 1 {
-                            self.good("...")?;
-                        }
-                    }
-                }
-            } else if let Some(num) = arg.num_vals {
-                let arg_name = format!("<{}>", arg.name);
-                let mut it = (0..num).peekable();
-                while it.next().is_some() {
-                    self.good(&arg_name)?;
-                    if it.peek().is_some() {
-                        self.none(&delim.to_string())?;
-                    }
-                }
-                if mult && num == 1 {
-                    self.good("...")?;
-                }
-            } else if !arg.is_positional() {
-                self.good(&format!("<{}>", arg.name))?;
-                if mult {
-                    self.good("...")?;
-                }
-            } else {
-                self.good(&arg.to_string())?;
-            }
+        if arg.is_set(ArgSettings::TakesValue) || arg.is_positional() {
+            display_arg_val(
+                arg,
+                |s, good| if good { self.good(s) } else { self.none(s) },
+            )?;
         }
 
         debug!("Help::val: Has switch...");

diff --git a/tests/help.rs b/tests/help.rs
@@ -186,8 +186,8 @@ FLAGS:
     -V, --version    Print version information
 
 OPTIONS:
-    -o, --option <option>...    tests options
-    -O, --opt <opt>             tests options";
+    -o, --option <option>    tests options
+    -O, --opt <opt>          tests options";
 
 static RIPGREP_USAGE: &str = "ripgrep 0.5
 
@@ -1247,6 +1247,34 @@ fn issue_760() {
     assert!(utils::compare_output(app, "ctest --help", ISSUE_760, false));
 }
 
+#[test]
+fn issue_1571() {
+    let app = App::new("hello").arg(
+        Arg::new("name")
+            .long("package")
+            .short('p')
+            .number_of_values(1)
+            .takes_value(true)
+            .multiple_values(true),
+    );
+    assert!(utils::compare_output(
+        app,
+        "hello --help",
+        "hello 
+
+USAGE:
+    hello [OPTIONS]
+
+FLAGS:
+    -h, --help       Print help information
+    -V, --version    Print version information
+
+OPTIONS:
+    -p, --package <name>    ",
+        false
+    ));
+}
+
 #[test]
 fn ripgrep_usage() {
     let app = App::new("ripgrep").version("0.5").override_usage(