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
    <name> [-b] [-a] ...

...
OPTIONS
    -b
    -a

instead of

...
SYNOPSIS
    <name> [-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.

Author: Eric Gumba

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| option_sort_key(opt));
+
+    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| 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()) {
             (Some(short), Some(long)) => {
                 vec![short_option(short), roman(", "), long_option(long)]
@@ -217,7 +224,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(|c| subcommand_sort_key(c));
+    for sub in sorted_subcommands {
         roff.control("TP", []);
 
         let name = format!(
@@ -378,3 +388,23 @@ fn format_possible_values(possibles: &Vec<&clap::builder::PossibleValue>) -> (Ve
     }
     (lines, with_help)
 }
+
+fn subcommand_sort_key(command: &clap::Command) -> (usize, &str) {
+    (command.get_display_order(), command.get_name())
+}
+
+/// Note that this function is duplicated from `clap::builder`
+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)
+}

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

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

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

clap_mangen/tests/testsuite/common.rs

@@ -273,6 +273,36 @@ 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,6 +354,40 @@ 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(
@@ -356,8 +420,7 @@ pub(crate) fn help_headings(name: &'static str) -> clap::Command {
 }
 
 pub(crate) fn value_with_required_equals(name: &'static str) -> clap::Command {
-    clap::Command::new(name)
-    .arg(
+    clap::Command::new(name).arg(
         clap::Arg::new("config")
             .long("config")
             .value_name("FILE")
@@ -367,8 +430,7 @@ pub(crate) fn value_with_required_equals(name: &'static str) -> clap::Command {
 }
 
 pub(crate) fn optional_value_with_required_equals(name: &'static str) -> clap::Command {
-    clap::Command::new(name)
-    .arg(
+    clap::Command::new(name).arg(
         clap::Arg::new("config")
             .long("config")
             .value_name("FILE")
@@ -379,8 +441,7 @@ pub(crate) fn optional_value_with_required_equals(name: &'static str) -> clap::C
 }
 
 pub(crate) fn optional_value(name: &'static str) -> clap::Command {
-    clap::Command::new(name)
-    .arg(
+    clap::Command::new(name).arg(
         clap::Arg::new("config")
             .long("config")
             .value_name("FILE")
@@ -390,8 +451,7 @@ pub(crate) fn optional_value(name: &'static str) -> clap::Command {
 }
 
 pub(crate) fn multiple_optional_values(name: &'static str) -> clap::Command {
-    clap::Command::new(name)
-    .arg(
+    clap::Command::new(name).arg(
         clap::Arg::new("config")
             .long("config")
             .value_names(["FILE1", "FILE2"])
@@ -401,13 +461,45 @@ pub(crate) fn multiple_optional_values(name: &'static str) -> clap::Command {
 }
 
 pub(crate) fn variadic_values(name: &'static str) -> clap::Command {
-    clap::Command::new(name)
-    .arg(
+    clap::Command::new(name).arg(
         clap::Arg::new("config")
             .long("config")
             .value_names(["FILE1", "FILE2"])
             .require_equals(false)
             .num_args(3)
             .help("Optional config file"),
     )
-}
+}
+
+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),
+        ),
+    ])
+}

clap_mangen/tests/testsuite/roff.rs

@@ -137,10 +137,7 @@ fn optional_value_with_required_equals() {
 fn optional_value() {
     let name = "my-app";
     let cmd = common::optional_value(name);
-    common::assert_matches(
-        snapbox::file!["../snapshots/optional_value.bash.roff"],
-        cmd,
-    );
+    common::assert_matches(snapbox::file!["../snapshots/optional_value.bash.roff"], cmd);
 }
 
 #[test]
@@ -162,3 +159,60 @@ fn variadic_values() {
         cmd,
     );
 }
+
+#[test]
+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,
+    );
+}