From 1d261e48c0ee62924cb0e81dd7f16b005e448845 Mon Sep 17 00:00:00 2001 From: Eric Gumba Date: Fri, 18 Jul 2025 23:45:38 -0700 Subject: [PATCH 1/3] test(clap_mangen): Test mangen display order --- .../configured_display_order_args.roff | 33 +++++++++++++++++ clap_mangen/tests/testsuite/common.rs | 37 +++++++++++++++++++ clap_mangen/tests/testsuite/roff.rs | 11 ++++++ 3 files changed, 81 insertions(+) create mode 100644 clap_mangen/tests/snapshots/configured_display_order_args.roff diff --git a/clap_mangen/tests/snapshots/configured_display_order_args.roff b/clap_mangen/tests/snapshots/configured_display_order_args.roff new file mode 100644 index 00000000000..3a5135308d7 --- /dev/null +++ b/clap_mangen/tests/snapshots/configured_display_order_args.roff @@ -0,0 +1,33 @@ +.ie \n(.g .ds Aq \(aq +.el .ds Aq ' +.TH my-app 1 "my-app " +.SH NAME +my\-app +.SH SYNOPSIS +\fBmy\-app\fR [\fB\-O\fR|\fB\-\-first\fR] [\fB\-P\fR|\fB\-\-second\fR] [\fB\-Q\fR|\fB\-\-third\fR] [\fB\-\-fourth\fR] [\fB\-h\fR|\fB\-\-help\fR] [\fI1st\fR] [\fI2nd\fR] [\fI3rd\fR] +.SH DESCRIPTION +.SH OPTIONS +.TP +\fB\-O\fR, \fB\-\-first\fR +Should be 1st +.TP +\fB\-P\fR, \fB\-\-second\fR +Should be 2nd +.TP +\fB\-Q\fR, \fB\-\-third\fR +Should be 3rd +.TP +\fB\-\-fourth\fR +Should be 4th +.TP +\fB\-h\fR, \fB\-\-help\fR +Print help +.TP +[\fI1st\fR] +1st +.TP +[\fI2nd\fR] +2nd +.TP +[\fI3rd\fR] +3rd diff --git a/clap_mangen/tests/testsuite/common.rs b/clap_mangen/tests/testsuite/common.rs index e4ed80c535b..358bfcbdaee 100644 --- a/clap_mangen/tests/testsuite/common.rs +++ b/clap_mangen/tests/testsuite/common.rs @@ -324,6 +324,43 @@ pub(crate) fn value_name_without_arg(name: &'static str) -> clap::Command { ) } + +pub(crate) fn configured_display_order_args(name: &'static str) -> clap::Command { + clap::Command::new(name) + .arg(clap::Arg::new("1st").help("1st")) + .arg(clap::Arg::new("2nd").help("2nd")) + .arg(clap::Arg::new("3rd").help("3rd").last(true)) + .arg( + clap::Arg::new("c") + .long("third") + .short('Q') + .display_order(3) + .help("Should be 3rd"), + ) + .arg( + clap::Arg::new("d") + .long("fourth") + .display_order(4) + .help("Should be 4th"), + ) + .arg( + clap::Arg::new("a") + .long("first") + .short('O') + .display_order(1) + .help("Should be 1st"), + ) + .arg( + clap::Arg::new("b") + .long("second") + .short('P') + .display_order(2) + .help("Should be 2nd"), + ) + +} + + pub(crate) fn help_headings(name: &'static str) -> clap::Command { clap::Command::new(name) .arg( diff --git a/clap_mangen/tests/testsuite/roff.rs b/clap_mangen/tests/testsuite/roff.rs index 6adfcd238fc..bea852ac686 100644 --- a/clap_mangen/tests/testsuite/roff.rs +++ b/clap_mangen/tests/testsuite/roff.rs @@ -112,3 +112,14 @@ fn value_name_without_arg() { cmd, ); } + +#[test] +fn configured_display_order_args() { + let name = "my-app"; + let cmd = common::configured_display_order_args(name); + + common::assert_matches( + snapbox::file!["../snapshots/configured_display_order_args.roff"], + cmd, + ); +} From 521f01d5bb5567a59b2bc48e66bcaa8713302110 Mon Sep 17 00:00:00 2001 From: Eric Gumba Date: Fri, 18 Jul 2025 23:47:06 -0700 Subject: [PATCH 2/3] fix(clap_mangen): Take into consideration display_order In #3362 we have an issue where when we configure an arg via .display_order(int), and then generate a manpage, the synposis and options will render the order the args were provided to the App rather than the order they were configured e.g Command::new(name) arg(Arg::new("few").short('b').display_order(2)) arg(Arg::new("bar").short('a').display_order(1)) will show ... SYNOPSIS [-b] [-a] ... ... OPTIONS -b -a instead of ... SYNOPSIS [-a] [-b] ... ... OPTIONS -a -b and so on. This fix adds sorting in the synopsis and options functions responsible for generating the corresponding synopsis and options sections of the manpage. --- clap_mangen/src/render.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/clap_mangen/src/render.rs b/clap_mangen/src/render.rs index c48d13a4812..976484f6769 100644 --- a/clap_mangen/src/render.rs +++ b/clap_mangen/src/render.rs @@ -33,7 +33,11 @@ pub(crate) fn synopsis(roff: &mut Roff, cmd: &clap::Command) { let name = cmd.get_bin_name().unwrap_or_else(|| cmd.get_name()); let mut line = vec![bold(name), roman(" ")]; - for opt in cmd.get_arguments().filter(|i| !i.is_hide_set()) { + let mut opts: Vec<_> = cmd.get_arguments().filter(|i| !i.is_hide_set()).collect(); + + opts.sort_by_key(|opt| opt.get_display_order()); + + for opt in opts { let (lhs, rhs) = option_markers(opt); match (opt.get_short(), opt.get_long()) { (Some(short), Some(long)) => { @@ -89,7 +93,10 @@ pub(crate) fn synopsis(roff: &mut Roff, cmd: &clap::Command) { } pub(crate) fn options(roff: &mut Roff, items: &[&Arg]) { - for opt in items.iter().filter(|a| !a.is_positional()) { + let mut sorted_items = items.to_vec(); + sorted_items.sort_by_key(|opt| opt.get_display_order()); + + for opt in sorted_items.iter().filter(|a| !a.is_positional()) { let mut header = match (opt.get_short(), opt.get_long()) { (Some(short), Some(long)) => { vec![short_option(short), roman(", "), long_option(long)] @@ -131,7 +138,7 @@ pub(crate) fn options(roff: &mut Roff, items: &[&Arg]) { } } - for pos in items.iter().filter(|a| a.is_positional()) { + for pos in sorted_items.iter().filter(|a| a.is_positional()) { let mut header = vec![]; let (lhs, rhs) = option_markers(pos); header.push(roman(lhs)); From 6fd3d69686fa314e71fe3c50a56c522159e7fbfa Mon Sep 17 00:00:00 2001 From: Eric Gumba Date: Thu, 24 Jul 2025 05:34:41 -0700 Subject: [PATCH 3/3] fix: Code review fixups 1. Reused sorting method in help_template.rs. 2. More thorough testing of configured ordering, instead of relying solely on snapbox. 3. Added sorting to subcommand section of clap_mangen --- clap_mangen/src/render.rs | 31 ++++++++-- .../snapshots/configured_subcmd_order.roff | 27 ++++++++ .../tests/snapshots/default_subcmd_order.roff | 27 ++++++++ clap_mangen/tests/testsuite/common.rs | 62 ++++++++++++++++++- clap_mangen/tests/testsuite/roff.rs | 50 +++++++++++++++ 5 files changed, 191 insertions(+), 6 deletions(-) create mode 100644 clap_mangen/tests/snapshots/configured_subcmd_order.roff create mode 100644 clap_mangen/tests/snapshots/default_subcmd_order.roff diff --git a/clap_mangen/src/render.rs b/clap_mangen/src/render.rs index 976484f6769..e1a903299f0 100644 --- a/clap_mangen/src/render.rs +++ b/clap_mangen/src/render.rs @@ -1,6 +1,26 @@ use clap::{Arg, ArgAction}; use roff::{bold, italic, roman, Inline, Roff}; +pub(crate) fn option_sort_key(arg: &Arg) -> (usize, String) { + let key = if let Some(x) = arg.get_short() { + let mut s = x.to_ascii_lowercase().to_string(); + s.push(if x.is_ascii_lowercase() { '0' } else { '1' }); + s + } else if let Some(x) = arg.get_long() { + x.to_string() + } else { + let mut s = '{'.to_string(); + s.push_str(arg.get_id().as_str()); + s + }; + (arg.get_display_order(), key) +} + +/// Provides consistent sorting for subcommands +pub(crate) fn subcmd_sort_key(subcmd: &clap::Command) -> (usize, &str) { + (subcmd.get_display_order(), subcmd.get_name()) +} + pub(crate) fn subcommand_heading(cmd: &clap::Command) -> &str { match cmd.get_subcommand_help_heading() { Some(title) => title, @@ -35,7 +55,7 @@ pub(crate) fn synopsis(roff: &mut Roff, cmd: &clap::Command) { let mut opts: Vec<_> = cmd.get_arguments().filter(|i| !i.is_hide_set()).collect(); - opts.sort_by_key(|opt| opt.get_display_order()); + opts.sort_by_key(|opt| option_sort_key(opt)); for opt in opts { let (lhs, rhs) = option_markers(opt); @@ -94,7 +114,7 @@ pub(crate) fn synopsis(roff: &mut Roff, cmd: &clap::Command) { pub(crate) fn options(roff: &mut Roff, items: &[&Arg]) { let mut sorted_items = items.to_vec(); - sorted_items.sort_by_key(|opt| opt.get_display_order()); + sorted_items.sort_by_key(|opt| option_sort_key(opt)); for opt in sorted_items.iter().filter(|a| !a.is_positional()) { let mut header = match (opt.get_short(), opt.get_long()) { @@ -138,7 +158,7 @@ pub(crate) fn options(roff: &mut Roff, items: &[&Arg]) { } } - for pos in sorted_items.iter().filter(|a| a.is_positional()) { + for pos in items.iter().filter(|a| a.is_positional()) { let mut header = vec![]; let (lhs, rhs) = option_markers(pos); header.push(roman(lhs)); @@ -206,7 +226,10 @@ fn possible_options(roff: &mut Roff, arg: &Arg, arg_help_written: bool) { } pub(crate) fn subcommands(roff: &mut Roff, cmd: &clap::Command, section: &str) { - for sub in cmd.get_subcommands().filter(|s| !s.is_hide_set()) { + let mut sorted_subcommands: Vec<_> = + cmd.get_subcommands().filter(|s| !s.is_hide_set()).collect(); + sorted_subcommands.sort_by_key(|sub| subcmd_sort_key(sub)); + for sub in sorted_subcommands { roff.control("TP", []); let name = format!( diff --git a/clap_mangen/tests/snapshots/configured_subcmd_order.roff b/clap_mangen/tests/snapshots/configured_subcmd_order.roff new file mode 100644 index 00000000000..b8128b96deb --- /dev/null +++ b/clap_mangen/tests/snapshots/configured_subcmd_order.roff @@ -0,0 +1,27 @@ +.ie \n(.g .ds Aq \(aq +.el .ds Aq ' +.TH my-app 1 "my-app 1" +.SH NAME +my\-app +.SH SYNOPSIS +\fBmy\-app\fR [\fB\-h\fR|\fB\-\-help\fR] [\fB\-V\fR|\fB\-\-version\fR] [\fIsubcommands\fR] +.SH DESCRIPTION +.SH OPTIONS +.TP +\fB\-h\fR, \fB\-\-help\fR +Print help +.TP +\fB\-V\fR, \fB\-\-version\fR +Print version +.SH SUBCOMMANDS +.TP +my\-app\-a1(1) +blah a1 +.TP +my\-app\-b1(1) +blah b1 +.TP +my\-app\-help(1) +Print this message or the help of the given subcommand(s) +.SH VERSION +v1 diff --git a/clap_mangen/tests/snapshots/default_subcmd_order.roff b/clap_mangen/tests/snapshots/default_subcmd_order.roff new file mode 100644 index 00000000000..f9ce5dbd7dc --- /dev/null +++ b/clap_mangen/tests/snapshots/default_subcmd_order.roff @@ -0,0 +1,27 @@ +.ie \n(.g .ds Aq \(aq +.el .ds Aq ' +.TH my-app 1 "my-app 1" +.SH NAME +my\-app +.SH SYNOPSIS +\fBmy\-app\fR [\fB\-h\fR|\fB\-\-help\fR] [\fB\-V\fR|\fB\-\-version\fR] [\fIsubcommands\fR] +.SH DESCRIPTION +.SH OPTIONS +.TP +\fB\-h\fR, \fB\-\-help\fR +Print help +.TP +\fB\-V\fR, \fB\-\-version\fR +Print version +.SH SUBCOMMANDS +.TP +my\-app\-b1(1) +blah b1 +.TP +my\-app\-a1(1) +blah a1 +.TP +my\-app\-help(1) +Print this message or the help of the given subcommand(s) +.SH VERSION +v1 diff --git a/clap_mangen/tests/testsuite/common.rs b/clap_mangen/tests/testsuite/common.rs index 358bfcbdaee..2bfb432131b 100644 --- a/clap_mangen/tests/testsuite/common.rs +++ b/clap_mangen/tests/testsuite/common.rs @@ -273,6 +273,39 @@ pub(crate) fn env_value_command(name: &'static str) -> clap::Command { ) } +pub(crate) fn mangen_output(cmd: &clap::Command) -> String { + let mut buf = vec![]; + clap_mangen::Man::new(cmd.clone()).render(&mut buf).unwrap(); + + let s = match std::str::from_utf8(&buf) { + Ok(s) => s, + Err(e) => panic!("Failed to convert output to UTF-8: {e}"), + }; + + s.to_string() +} + +// Go through the output and assert that the keywords +// appear in the expected order +pub(crate) fn is_correct_ordering( + keywords: &[&'static str], + text: &str, +) -> bool { + let mut s = text; + for keyword in keywords { + if !s.contains(keyword) { + return false; + } + // everytime we find a match, + // we only look at the rest of the string + s = match s.split(keyword).last() { + Some(rest) => rest, + None => return false, + }; + } + true +} + pub(crate) fn assert_matches(expected: impl IntoData, cmd: clap::Command) { let mut buf = vec![]; clap_mangen::Man::new(cmd).render(&mut buf).unwrap(); @@ -324,7 +357,6 @@ pub(crate) fn value_name_without_arg(name: &'static str) -> clap::Command { ) } - pub(crate) fn configured_display_order_args(name: &'static str) -> clap::Command { clap::Command::new(name) .arg(clap::Arg::new("1st").help("1st")) @@ -360,7 +392,6 @@ pub(crate) fn configured_display_order_args(name: &'static str) -> clap::Command } - pub(crate) fn help_headings(name: &'static str) -> clap::Command { clap::Command::new(name) .arg( @@ -391,3 +422,30 @@ pub(crate) fn help_headings(name: &'static str) -> clap::Command { .value_parser(["always", "never", "auto"]), ) } + +pub(crate) fn configured_subcmd_order(name: &'static str) -> clap::Command { + clap::Command::new(name) + .version("1") + .next_display_order(None) + .subcommands(vec![ + clap::Command::new("b1") + .about("blah b1") + .arg(clap::Arg::new("test").short('t').action(clap::ArgAction::SetTrue)), + clap::Command::new("a1") + .about("blah a1") + .arg(clap::Arg::new("roster").short('r').action(clap::ArgAction::SetTrue)), + ]) +} + +pub(crate) fn default_subcmd_order(name: &'static str) -> clap::Command { + clap::Command::new(name) + .version("1") + .subcommands(vec![ + clap::Command::new("b1") + .about("blah b1") + .arg(clap::Arg::new("test").short('t').action(clap::ArgAction::SetTrue)), + clap::Command::new("a1") + .about("blah a1") + .arg(clap::Arg::new("roster").short('r').action(clap::ArgAction::SetTrue)), + ]) +} \ No newline at end of file diff --git a/clap_mangen/tests/testsuite/roff.rs b/clap_mangen/tests/testsuite/roff.rs index bea852ac686..34c2365633b 100644 --- a/clap_mangen/tests/testsuite/roff.rs +++ b/clap_mangen/tests/testsuite/roff.rs @@ -118,8 +118,58 @@ fn configured_display_order_args() { let name = "my-app"; let cmd = common::configured_display_order_args(name); + let s = common::mangen_output(&cmd); + + let ordered_keywords = [ + "first", "second", "third", "fourth", "1st", "2nd", "3rd", + ]; + let default_ordered_keywords = [ + "third", "fourth", "first", "second", "1st", "2nd", "3rd", + ]; + + assert!(common::is_correct_ordering(&ordered_keywords, &s)); + assert!(!common::is_correct_ordering(&default_ordered_keywords, &s)); + common::assert_matches( snapbox::file!["../snapshots/configured_display_order_args.roff"], cmd, ); } + +#[test] +fn configured_subcmd_order() { + let name = "my-app"; + let cmd = common::configured_subcmd_order(name); + + let s = common::mangen_output(&cmd); + + let ordered_keywords = ["a1", "b1"]; + let default_ordered_keywords = ["b1", "a1"]; + + assert!(common::is_correct_ordering(&ordered_keywords, &s)); + assert!(!common::is_correct_ordering(&default_ordered_keywords, &s)); + + common::assert_matches( + snapbox::file!["../snapshots/configured_subcmd_order.roff"], + cmd, + ); +} + +#[test] +fn default_subcmd_order() { + let name = "my-app"; + let cmd = common::default_subcmd_order(name); + + let s = common::mangen_output(&cmd); + + let ordered_keywords = ["a1", "b1"]; + let default_ordered_keywords = ["b1", "a1"]; + + assert!(!common::is_correct_ordering(&ordered_keywords, &s)); + assert!(common::is_correct_ordering(&default_ordered_keywords, &s)); + + common::assert_matches( + snapbox::file!["../snapshots/default_subcmd_order.roff"], + cmd, + ); +} \ No newline at end of file