From 33949ce4de61826d9295624e9ee6fac6a2433932 Mon Sep 17 00:00:00 2001 From: Roland Fredenhagen Date: Wed, 2 Mar 2022 16:15:28 +0100 Subject: [PATCH] feat(help): Show PossibleValue help in `--help` (#3503) `-h` (short help) still shows the same. This gates it behind an `unstable-v4` feature flag to avoid disrupting users who set the help without knowing where all it shows up (particularly derive users where `ArgEnum` is automatically extracting the help). Fixes #3312 --- .github/workflows/ci.yml | 6 +- .github/workflows/rust-next.yml | 6 +- Cargo.toml | 1 + Makefile | 1 + README.md | 1 + clap_complete/src/shells/bash.rs | 3 +- clap_complete/src/shells/zsh.rs | 3 +- src/build/possible_value.rs | 37 +++++++- src/output/help.rs | 130 +++++++++++++++++++++------ src/parse/parser.rs | 8 +- src/parse/validator.rs | 12 ++- tests/builder/help.rs | 149 +++++++++++++++++++++++++++++++ tests/builder/utils.rs | 23 ++++- 13 files changed, 340 insertions(+), 40 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index a224a3c4b6b..9c91fbad005 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -28,7 +28,7 @@ jobs: name: Test strategy: matrix: - build: [linux, windows, mac, minimal, default] + build: [linux, windows, mac, minimal, default, next] include: - build: linux os: ubuntu-latest @@ -50,6 +50,10 @@ jobs: os: ubuntu-latest rust: "stable" features: "default" + - build: next + os: ubuntu-latest + rust: "stable" + features: "next" continue-on-error: ${{ matrix.rust != 'stable' }} runs-on: ${{ matrix.os }} steps: diff --git a/.github/workflows/rust-next.yml b/.github/workflows/rust-next.yml index f66a64970b3..50a46298721 100644 --- a/.github/workflows/rust-next.yml +++ b/.github/workflows/rust-next.yml @@ -7,7 +7,7 @@ jobs: name: Test strategy: matrix: - build: [stable, linux, windows, mac, nightly, minimal, default] + build: [stable, linux, windows, mac, nightly, minimal, default, next] include: - build: stable os: ubuntu-latest @@ -37,6 +37,10 @@ jobs: os: ubuntu-latest rust: "stable" features: "default" + - build: next + os: ubuntu-latest + rust: "stable" + features: "next" continue-on-error: ${{ matrix.rust != 'stable' }} runs-on: ${{ matrix.os }} steps: diff --git a/Cargo.toml b/Cargo.toml index 8a3754fabd1..7f5b6d096f4 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -81,6 +81,7 @@ unicode = ["textwrap/unicode-width", "unicase"] # Support for unicode character unstable-replace = [] unstable-multicall = [] unstable-grouped = [] +unstable-v4 = [] [lib] bench = false diff --git a/Makefile b/Makefile index 35f188b37fa..d428be77c37 100644 --- a/Makefile +++ b/Makefile @@ -17,6 +17,7 @@ _FEATURES_minimal = --no-default-features --features "std" _FEATURES_default = _FEATURES_wasm = --features "derive cargo env unicode yaml regex unstable-replace unstable-multicall unstable-grouped" _FEATURES_full = --features "derive cargo env unicode yaml regex unstable-replace unstable-multicall unstable-grouped wrap_help" +_FEATURES_next = ${_FEATURES_full} --features unstable-v4 _FEATURES_debug = ${_FEATURES_full} --features debug _FEATURES_release = ${_FEATURES_full} --release diff --git a/README.md b/README.md index 64b0799bb33..f3a0b850c3c 100644 --- a/README.md +++ b/README.md @@ -153,6 +153,7 @@ Why use the procedural [Builder API](https://github.com/clap-rs/clap/blob/v3.1.3 * **unstable-replace**: Enable [`Command::replace`](https://github.com/clap-rs/clap/issues/2836) * **unstable-multicall**: Enable [`Command::multicall`](https://github.com/clap-rs/clap/issues/2861) * **unstable-grouped**: Enable [`ArgMatches::grouped_values_of`](https://github.com/clap-rs/clap/issues/2924) +* **unstable-v4**: Preview features which will be stable on the v4.0 release ## Sponsors diff --git a/clap_complete/src/shells/bash.rs b/clap_complete/src/shells/bash.rs index 1625d0ce9c0..08bf1190cc3 100644 --- a/clap_complete/src/shells/bash.rs +++ b/clap_complete/src/shells/bash.rs @@ -178,7 +178,8 @@ fn vals_for(o: &Arg) -> String { format!( "$(compgen -W \"{}\" -- \"${{cur}}\")", vals.iter() - .filter_map(PossibleValue::get_visible_name) + .filter(|pv| pv.is_hide_set()) + .map(PossibleValue::get_name) .collect::>() .join(" ") ) diff --git a/clap_complete/src/shells/zsh.rs b/clap_complete/src/shells/zsh.rs index 2b785c60c7e..2b64739cedf 100644 --- a/clap_complete/src/shells/zsh.rs +++ b/clap_complete/src/shells/zsh.rs @@ -387,7 +387,8 @@ fn value_completion(arg: &Arg) -> Option { "({})", values .iter() - .filter_map(PossibleValue::get_visible_name) + .filter(|pv| !pv.is_hide_set()) + .map(PossibleValue::get_name) .collect::>() .join(" ") )) diff --git a/src/build/possible_value.rs b/src/build/possible_value.rs index bf611aa9523..e56fb858ce6 100644 --- a/src/build/possible_value.rs +++ b/src/build/possible_value.rs @@ -1,4 +1,4 @@ -use std::iter; +use std::{borrow::Cow, iter}; use crate::util::eq_ignore_case; @@ -148,6 +148,18 @@ impl<'help> PossibleValue<'help> { self.help } + /// Get the help specified for this argument, if any and the argument + /// value is not hidden + #[inline] + #[cfg(feature = "unstable-v4")] + pub(crate) fn get_visible_help(&self) -> Option<&'help str> { + if !self.hide { + self.help + } else { + None + } + } + /// Deprecated, replaced with [`PossibleValue::is_hide_set`] #[inline] #[deprecated(since = "3.1.0", note = "Replaced with `PossibleValue::is_hide_set`")] @@ -161,7 +173,16 @@ impl<'help> PossibleValue<'help> { self.hide } + /// Report if PossibleValue is not hidden and has a help message + pub(crate) fn should_show_help(&self) -> bool { + !self.hide && self.help.is_some() + } + /// Get the name if argument value is not hidden, `None` otherwise + #[deprecated( + since = "3.1.4", + note = "Use `PossibleValue::is_hide_set` and `PossibleValue::get_name`" + )] pub fn get_visible_name(&self) -> Option<&'help str> { if self.hide { None @@ -170,6 +191,20 @@ impl<'help> PossibleValue<'help> { } } + /// Get the name if argument value is not hidden, `None` otherwise, + /// but wrapped in quotes if it contains whitespace + pub(crate) fn get_visible_quoted_name(&self) -> Option> { + if !self.hide { + Some(if self.name.contains(char::is_whitespace) { + format!("{:?}", self.name).into() + } else { + self.name.into() + }) + } else { + None + } + } + /// Returns all valid values of the argument value. /// /// Namely the name and all aliases. diff --git a/src/output/help.rs b/src/output/help.rs index 7f5fb3bd7bc..03221686714 100644 --- a/src/output/help.rs +++ b/src/output/help.rs @@ -11,6 +11,7 @@ use std::{ use crate::{ build::{display_arg_val, Arg, Command}, output::{fmt::Colorizer, Usage}, + PossibleValue, }; // Third party @@ -385,7 +386,7 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> { /// Writes argument's help to the wrapped stream. fn help( &mut self, - is_not_positional: bool, + arg: Option<&Arg<'help>>, about: &str, spec_vals: &str, next_line_help: bool, @@ -401,7 +402,7 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> { longest + 12 }; - let too_long = spaces + display_width(about) + display_width(spec_vals) >= self.term_w; + let too_long = spaces + display_width(&help) >= self.term_w; // Is help on next line, if so then indent if next_line_help { @@ -423,17 +424,100 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> { if let Some(part) = help.lines().next() { self.none(part)?; } + + // indent of help + let spaces = if next_line_help { + TAB_WIDTH * 3 + } else if let Some(true) = arg.map(|a| a.is_positional()) { + longest + TAB_WIDTH * 2 + } else { + longest + TAB_WIDTH * 3 + }; + for part in help.lines().skip(1) { self.none("\n")?; - if next_line_help { - self.none(format!("{}{}{}", TAB, TAB, TAB))?; - } else if is_not_positional { - self.spaces(longest + 12)?; - } else { - self.spaces(longest + 8)?; - } + self.spaces(spaces)?; self.none(part)?; } + + #[cfg(feature = "unstable-v4")] + if let Some(arg) = arg { + const DASH_SPACE: usize = "- ".len(); + const COLON_SPACE: usize = ": ".len(); + if self.use_long + && !arg.is_hide_possible_values_set() + && arg + .possible_vals + .iter() + .any(PossibleValue::should_show_help) + { + debug!("Help::help: Found possible vals...{:?}", arg.possible_vals); + if !help.is_empty() { + self.none("\n\n")?; + self.spaces(spaces)?; + } + self.none("Possible values:")?; + let longest = arg + .possible_vals + .iter() + .filter_map(|f| f.get_visible_quoted_name().map(|name| display_width(&name))) + .max() + .expect("Only called with possible value"); + let help_longest = arg + .possible_vals + .iter() + .filter_map(|f| f.get_visible_help().map(display_width)) + .max() + .expect("Only called with possible value with help"); + // should new line + let taken = longest + spaces + DASH_SPACE; + + let possible_value_new_line = + self.term_w >= taken && self.term_w < taken + COLON_SPACE + help_longest; + + let spaces = spaces + TAB_WIDTH - DASH_SPACE; + let spaces_help = if possible_value_new_line { + spaces + DASH_SPACE + } else { + spaces + longest + DASH_SPACE + COLON_SPACE + }; + + for pv in arg.possible_vals.iter().filter(|pv| !pv.is_hide_set()) { + self.none("\n")?; + self.spaces(spaces)?; + self.none("- ")?; + self.good(pv.get_name())?; + if let Some(help) = pv.get_help() { + debug!("Help::help: Possible Value help"); + + if possible_value_new_line { + self.none(":\n")?; + self.spaces(spaces_help)?; + } else { + self.none(": ")?; + // To align help messages + self.spaces(longest - display_width(pv.get_name()))?; + } + + let avail_chars = if self.term_w > spaces_help { + self.term_w - spaces_help + } else { + usize::MAX + }; + + let help = text_wrapper(help, avail_chars); + let mut help = help.lines(); + + self.none(help.next().unwrap_or_default())?; + for part in help { + self.none("\n")?; + self.spaces(spaces_help)?; + self.none(part)?; + } + } + } + } + } Ok(()) } @@ -456,13 +540,7 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> { arg.help.or(arg.long_help).unwrap_or("") }; - self.help( - !arg.is_positional(), - about, - spec_vals, - next_line_help, - longest, - )?; + self.help(Some(arg), about, spec_vals, next_line_help, longest)?; Ok(()) } @@ -572,7 +650,12 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> { } } - if !a.is_hide_possible_values_set() && !a.possible_vals.is_empty() { + if !(a.is_hide_possible_values_set() + || a.possible_vals.is_empty() + || cfg!(feature = "unstable-v4") + && self.use_long + && a.possible_vals.iter().any(PossibleValue::should_show_help)) + { debug!( "Help::spec_vals: Found possible vals...{:?}", a.possible_vals @@ -581,15 +664,7 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> { let pvs = a .possible_vals .iter() - .filter_map(|value| { - if value.is_hide_set() { - None - } else if value.get_name().contains(char::is_whitespace) { - Some(format!("{:?}", value.get_name())) - } else { - Some(value.get_name().to_string()) - } - }) + .filter_map(PossibleValue::get_visible_quoted_name) .collect::>() .join(", "); @@ -670,7 +745,7 @@ impl<'help, 'cmd, 'writer> Help<'help, 'cmd, 'writer> { .unwrap_or(""); self.subcmd(sc_str, next_line_help, longest)?; - self.help(false, about, spec_vals, next_line_help, longest) + self.help(None, about, spec_vals, next_line_help, longest) } fn sc_spec_vals(&self, a: &Command) -> String { @@ -1011,6 +1086,7 @@ pub(crate) fn dimensions() -> Option<(usize, usize)> { } const TAB: &str = " "; +const TAB_WIDTH: usize = 4; pub(crate) enum HelpWriter<'writer> { Normal(&'writer mut dyn Write), diff --git a/src/parse/parser.rs b/src/parse/parser.rs index 9321b2822c5..0b515b51a53 100644 --- a/src/parse/parser.rs +++ b/src/parse/parser.rs @@ -8,7 +8,6 @@ use std::{ use os_str_bytes::RawOsStr; // Internal -use crate::build::AppSettings as AS; use crate::build::{Arg, Command}; use crate::error::Error as ClapError; use crate::error::Result as ClapResult; @@ -18,6 +17,7 @@ use crate::parse::features::suggestions; use crate::parse::{ArgMatcher, SubCommand}; use crate::parse::{Validator, ValueSource}; use crate::util::{color::ColorChoice, Id}; +use crate::{build::AppSettings as AS, PossibleValue}; use crate::{INTERNAL_ERROR_MSG, INVALID_UTF8}; pub(crate) struct Parser<'help, 'cmd> { @@ -786,7 +786,11 @@ impl<'help, 'cmd> Parser<'help, 'cmd> { // specified by the user is sent through. If hide_short_help is not included, // then items specified with hidden_short_help will also be hidden. let should_long = |v: &Arg| { - v.long_help.is_some() || v.is_hide_long_help_set() || v.is_hide_short_help_set() + v.long_help.is_some() + || v.is_hide_long_help_set() + || v.is_hide_short_help_set() + || cfg!(feature = "unstable-v4") + && v.possible_vals.iter().any(PossibleValue::should_show_help) }; // Subcommands aren't checked because we prefer short help for them, deferring to diff --git a/src/parse/validator.rs b/src/parse/validator.rs index 18e1bb7609e..e1d17ac2843 100644 --- a/src/parse/validator.rs +++ b/src/parse/validator.rs @@ -46,7 +46,8 @@ impl<'help, 'cmd, 'parser> Validator<'help, 'cmd, 'parser> { self.p.cmd, &o.possible_vals .iter() - .filter_map(PossibleValue::get_visible_name) + .filter(|pv| !pv.is_hide_set()) + .map(PossibleValue::get_name) .collect::>(), o, Usage::new(self.p.cmd) @@ -141,7 +142,8 @@ impl<'help, 'cmd, 'parser> Validator<'help, 'cmd, 'parser> { val_str.into_owned(), &arg.possible_vals .iter() - .filter_map(PossibleValue::get_visible_name) + .filter(|pv| !pv.is_hide_set()) + .map(PossibleValue::get_name) .collect::>(), arg, Usage::new(self.p.cmd) @@ -156,7 +158,8 @@ impl<'help, 'cmd, 'parser> Validator<'help, 'cmd, 'parser> { self.p.cmd, &arg.possible_vals .iter() - .filter_map(PossibleValue::get_visible_name) + .filter(|pv| !pv.is_hide_set()) + .map(PossibleValue::get_name) .collect::>(), arg, Usage::new(self.p.cmd) @@ -454,7 +457,8 @@ impl<'help, 'cmd, 'parser> Validator<'help, 'cmd, 'parser> { self.p.cmd, &a.possible_vals .iter() - .filter_map(PossibleValue::get_visible_name) + .filter(|pv| !pv.is_hide_set()) + .map(PossibleValue::get_name) .collect::>(), a, Usage::new(self.p.cmd) diff --git a/tests/builder/help.rs b/tests/builder/help.rs index d07829a26eb..211f7e0d483 100644 --- a/tests/builder/help.rs +++ b/tests/builder/help.rs @@ -964,6 +964,89 @@ OPTIONS: )); } +#[test] +#[cfg(all(feature = "wrap_help"))] +fn possible_value_wrapped_help() { + #[cfg(feature = "unstable-v4")] + static WRAPPED_HELP: &str = "test + +USAGE: + test [OPTIONS] + +OPTIONS: + -h, --help + Print help information + + --possible-values + Possible values: + - short_name: + Long enough help message, barely warrant wrapping + - second: + Short help gets handled the same + + --possible-values-with-new-line + Possible values: + - long enough name to trigger new line: + Really long enough help message to clearly warrant + wrapping believe me + - second + + --possible-values-without-new-line + Possible values: + - name: Short enough help message with no wrapping + - second: short help +"; + #[cfg(not(feature = "unstable-v4"))] + static WRAPPED_HELP: &str = r#"test + +USAGE: + test [OPTIONS] + +OPTIONS: + -h, --help Print help information + --possible-values [possible values: short_name, second] + --possible-values-with-new-line [possible values: "long enough name to trigger new line", second] + --possible-values-without-new-line [possible values: name, second] +"#; + let cmd = Command::new("test") + .term_width(67) + .arg( + Arg::new("possible_values") + .long("possible-values") + .possible_value( + PossibleValue::new("short_name") + .help("Long enough help message, barely warrant wrapping"), + ) + .possible_value( + PossibleValue::new("second").help("Short help gets handled the same"), + ), + ) + .arg( + Arg::new("possible_values_with_new_line") + .long("possible-values-with-new-line") + .possible_value( + PossibleValue::new("long enough name to trigger new line").help( + "Really long enough help message to clearly warrant wrapping believe me", + ), + ) + .possible_value("second"), + ) + .arg( + Arg::new("possible_values_without_new_line") + .long("possible-values-without-new-line") + .possible_value( + PossibleValue::new("name").help("Short enough help message with no wrapping"), + ) + .possible_value(PossibleValue::new("second").help("short help")), + ); + assert!(utils::compare_output( + cmd, + "test --help", + WRAPPED_HELP, + false + )); +} + #[test] fn complex_subcommand_help_output() { let a = utils::complex_app(); @@ -1061,6 +1144,72 @@ fn hide_single_possible_val() { )); } +#[test] +fn possible_vals_with_help() { + #[cfg(feature = "unstable-v4")] + static POS_VALS_HELP: &str = "ctest 0.1 + +USAGE: + ctest [OPTIONS] + +OPTIONS: + -c, --cafe + A coffeehouse, coffee shop, or café. + + -h, --help + Print help information + + -p, --pos + Some vals + + Possible values: + - fast + - slow: not as fast + + -V, --version + Print version information +"; + #[cfg(not(feature = "unstable-v4"))] + static POS_VALS_HELP: &str = "ctest 0.1 + +USAGE: + ctest [OPTIONS] + +OPTIONS: + -c, --cafe A coffeehouse, coffee shop, or café. + -h, --help Print help information + -p, --pos Some vals [possible values: fast, slow] + -V, --version Print version information +"; + let app = Command::new("ctest") + .version("0.1") + .arg( + Arg::new("pos") + .short('p') + .long("pos") + .value_name("VAL") + .possible_value("fast") + .possible_value(PossibleValue::new("slow").help("not as fast")) + .possible_value(PossibleValue::new("secret speed").hide(true)) + .help("Some vals") + .takes_value(true), + ) + .arg( + Arg::new("cafe") + .short('c') + .long("cafe") + .value_name("FILE") + .help("A coffeehouse, coffee shop, or café.") + .takes_value(true), + ); + assert!(utils::compare_output( + app, + "ctest --help", + POS_VALS_HELP, + false + )); +} + #[test] fn issue_626_panic() { let cmd = Command::new("ctest") diff --git a/tests/builder/utils.rs b/tests/builder/utils.rs index 5a87684382b..1e146ea4056 100644 --- a/tests/builder/utils.rs +++ b/tests/builder/utils.rs @@ -1,6 +1,6 @@ #![allow(unused_imports, dead_code)] -use std::io::{Cursor, Write}; +use std::io::{BufRead, Cursor, Write}; use std::str; use regex::Regex; @@ -19,6 +19,20 @@ where let left_ = re.replace_all(&*ls, ""); let right = re.replace_all(&*rs, ""); let b = left_ == right; + let line_diff = left_ + .lines() + .zip(right.lines()) + .enumerate() + .filter_map(|(line, (left, right))| { + if left != right { + Some(format!("Line {}:\n{}\n{}", line, left, right)) + } else { + None + } + }) + .collect::>() + .join("\n"); + let line_count_diff = (left_.lines().count() as isize) - right.lines().count() as isize; if !b { dbg!(&left_); dbg!(&right); @@ -27,7 +41,12 @@ where println!("{}", left_); println!("--> right"); println!("{}", right); - println!("--") + println!("--> diff"); + println!("{}", line_diff); + println!("--"); + if line_count_diff != 0 { + println!("left line count - right line count = {}", line_count_diff); + } } b }