From 0394ae6102d3b1c3ffc0bab17c9bb6317828b239 Mon Sep 17 00:00:00 2001 From: Donough Liu Date: Tue, 17 Aug 2021 17:17:18 +0800 Subject: [PATCH] Fix handling of `number_of_values` when displaying arg (#2701) * Remove dead logic * Outline common logic * Fix issue 1571 --- src/build/arg/mod.rs | 170 ++++++++++++++++++++----------------------- src/output/help.rs | 65 ++--------------- tests/help.rs | 32 +++++++- 3 files changed, 115 insertions(+), 152 deletions(-) diff --git a/src/build/arg/mod.rs b/src/build/arg/mod.rs index 1db343fe545..2608e52d538 100644 --- 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::>() - .join(&*delim) - )?; - } else { - write!(f, "<{}>", self.name)?; +/// Write the values such as +pub(crate) fn display_arg_val(arg: &Arg, mut write: F) -> Result<(), E> +where + F: FnMut(&str, bool) -> Result, +{ + 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 - 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 index 7e118d04aec..dcfab1249d4 100644 --- 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 index 784e91de4ea..d06ae46d632 100644 --- a/tests/help.rs +++ b/tests/help.rs @@ -186,8 +186,8 @@ FLAGS: -V, --version Print version information OPTIONS: - -o, --option