responded to code review:

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

Author: Eric Gumba

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!(

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,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)),
+    ])
+}

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,
+    );
+}