diff --git a/argv/src/help.rs b/argv/src/help.rs index 97a64eaf1..3559926a4 100644 --- a/argv/src/help.rs +++ b/argv/src/help.rs @@ -469,7 +469,17 @@ fn column_usage(meta: &FlagMeta<'_>) -> String { return rest; }; let (before, after) = rest.split_at(at); - let short = match before.trim() { + let short = before.trim(); + // Only a bare short form belongs in the short column. A flag may carry a declared name the + // forms do not imply — `jobs: -j --parallel` — and that prefix is not something to line up + // with a comma after: it rendered `jobs: -j,--parallel`, losing the space entirely, because + // the glued string is already wider than the column. + let bare_short = short.is_empty() + || (short.starts_with('-') && !short.starts_with("--") && short.chars().count() == 2); + if !bare_short { + return rest; + } + let short = match short { "" => String::new(), s => format!("{s},"), }; diff --git a/conformance/tests/flag_column.rs b/conformance/tests/flag_column.rs index bb952ecd5..036f4ec8a 100644 --- a/conformance/tests/flag_column.rs +++ b/conformance/tests/flag_column.rs @@ -131,3 +131,47 @@ fn the_fields_are_bound() { assert!(ex.github_release && ex.dry_run && ex.describe); assert_eq!(ex.output.as_deref(), Some("o")); } + +/// A flag whose declared name the forms do not imply, and one whose help says nothing +#[derive(Cli)] +#[usage(bin = "odd")] +struct Odd { + /// How many at once + #[usage(name = "jobs", long = "parallel", short = 'j')] + parallel: Option, + /// A description made only of spaces is no description + #[usage(long, help = " ")] + blank: bool, +} + +#[test] +fn a_declared_name_is_not_mistaken_for_a_short_form() { + // `jobs: -j --parallel` — the prefix is the flag's *name*, not something to line a comma up + // after. Gluing one on lost the space entirely and rendered `jobs: -j,--parallel`, because + // the joined string is already wider than the column it was being padded to. + let page = usage_argv::help::render(Odd::spec(), Odd::spec().root.cmd, false).expect("a page"); + assert!(page.contains("jobs: -j --parallel"), "{page}"); + assert!(!page.contains("-j,--parallel"), "{page}"); +} + +#[test] +fn a_description_of_only_spaces_is_no_description() { + // Filtered wherever a description is read, so it does not buy a column of padding and a + // line of trailing spaces. usage-lib normalises it in the docs model for the same reason — + // one blank spec, two renderings, was a parity break waiting to be found. + let page = usage_argv::help::render(Odd::spec(), Odd::spec().root.cmd, false).expect("a page"); + let line = page + .lines() + .find(|l| l.contains("--blank")) + .unwrap_or_else(|| panic!("{page}")); + assert_eq!(line, line.trim_end(), "trailing space on {line:?}"); +} + +#[test] +fn the_odd_fields_are_bound() { + use std::ffi::OsStr; + let argv = ["-j", "4", "--blank"].map(OsStr::new); + let odd = Odd::parse_from(&argv).expect("should parse"); + assert_eq!(odd.parallel.as_deref(), Some("4")); + assert!(odd.blank); +} diff --git a/lib/src/docs/cli/mod.rs b/lib/src/docs/cli/mod.rs index 5d6fbd1d9..05e4417de 100644 --- a/lib/src/docs/cli/mod.rs +++ b/lib/src/docs/cli/mod.rs @@ -84,6 +84,38 @@ mod tests { use super::*; use insta::assert_snapshot; + #[test] + fn a_description_of_only_spaces_is_no_description() { + // `usage-argv` filters a blank description wherever it reads one, and this template + // asked only whether the string was there — so `help=" "` bought a column of padding + // and a line of trailing spaces here and nothing there. Two renderings of one spec. + // + // Asserted on the trailing whitespace rather than by comparing the two renderers, so + // the test says what is wrong with the line rather than only that they disagree. + let spec = crate::spec! { r#" +bin "ex" +flag "--blank" help=" " +flag "--plain" help="plain" + "# } + .unwrap(); + + for long in [false, true] { + let page = super::render_help(&spec, &spec.cmd, long); + // In the flags section, not the usage line — `Usage: ex [--blank] [--plain]` + // also contains the name and has no padding to get wrong. + let listing = page.split_once("\nFlags:").expect("a flags section").1; + let line = listing + .lines() + .find(|l| l.contains("--blank")) + .unwrap_or_else(|| panic!("long={long}: {page}")); + assert_eq!( + line, + line.trim_end(), + "long={long}: trailing space on {line:?}" + ); + } + } + #[test] fn test_render_help_omits_hidden_entries() { let spec = crate::spec! { r#" diff --git a/lib/src/docs/models.rs b/lib/src/docs/models.rs index 20e6e6836..5770f7dcc 100644 --- a/lib/src/docs/models.rs +++ b/lib/src/docs/models.rs @@ -521,6 +521,16 @@ impl From<&crate::SpecCommand> for SpecCommand { } } +/// Help text, with whitespace-only treated as none. +/// +/// `usage-argv` filters a blank description out everywhere it reads one, so a spec written with +/// `help=" "` produced a padded column and a line of trailing spaces here and nothing there — +/// two renderings of the same metadata. Normalised once, where the model is built, so every +/// renderer downstream sees the same answer. +fn said(help: &Option) -> Option { + help.as_ref().filter(|h| !h.trim().is_empty()).cloned() +} + /// The width of the short column: `-x, `, or the blank that stands in for it. /// /// Fixed, because a short form is one character. clap's, measured. @@ -560,7 +570,16 @@ fn column_usage(flag: &crate::SpecFlag) -> String { return rest; }; let (before, after) = rest.split_at(at); - let short = match before.trim() { + let short = before.trim(); + // Only a bare short form belongs in the short column — see the twin in `usage-argv`. A + // declared name the forms do not imply (`jobs: -j --parallel`) is not one, and gluing a + // comma to it lost the space before the long form. + let bare_short = short.is_empty() + || (short.starts_with('-') && !short.starts_with("--") && short.chars().count() == 2); + if !bare_short { + return rest; + } + let short = match short { "" => String::new(), s => format!("{s},"), }; @@ -574,7 +593,7 @@ impl From<&crate::SpecFlag> for SpecFlag { effect: flag.effect, usage: flag.usage.clone(), display_usage: column_usage(flag), - help: flag.help.clone(), + help: said(&flag.help), help_long: flag.help_long.clone(), help_md: flag.help_md.clone(), help_first_line: flag.help_first_line.clone(), @@ -618,7 +637,7 @@ impl From<&crate::SpecArg> for SpecArg { Self { name: arg.name.clone(), usage: arg.usage.clone(), - help: arg.help.clone(), + help: said(&arg.help), help_long: arg.help_long.clone(), help_md: arg.help_md.clone(), help_first_line: arg.help_first_line.clone(),