diff --git a/src/commands/completions.rs b/src/commands/completions.rs index 64672096..c1f36816 100644 --- a/src/commands/completions.rs +++ b/src/commands/completions.rs @@ -1,14 +1,13 @@ use anyhow::{Context, Result}; -use clap::CommandFactory; use clap_complete::Shell; use std::fs; use std::path::PathBuf; -use crate::Cli; +use crate::cli_command; /// Generate completions to stdout (the raw clap output). pub fn generate(shell: Shell) { - clap_complete::generate(shell, &mut Cli::command(), "pup", &mut std::io::stdout()); + clap_complete::generate(shell, &mut cli_command(), "pup", &mut std::io::stdout()); } /// Install a dynamic loader script for the given shell. diff --git a/src/main.rs b/src/main.rs index a036b17c..9fff5a90 100644 --- a/src/main.rs +++ b/src/main.rs @@ -81,6 +81,12 @@ pub(crate) struct Cli { command: Commands, } +/// Build the CLI with top-level subcommands sorted by name in help output. +/// Nested subcommands retain their declared display order. +pub(crate) fn cli_command() -> clap::Command { + Cli::command().mut_subcommands(|command| command.display_order(0)) +} + #[derive(Subcommand)] enum Commands { /// Start a local ACP server that proxies to Datadog Bits AI @@ -13333,7 +13339,7 @@ async fn main_inner() -> anyhow::Result<()> { let has_agent_flag = args.iter().any(|a| a == "--agent"); let has_no_agent_flag = args.iter().any(|a| a == "--no-agent"); if has_help && !has_no_agent_flag && (useragent::is_agent_mode() || has_agent_flag) { - let cmd = Cli::command(); + let cmd = cli_command(); if let Some(schema) = agent_help_schema(&cmd, &args) { println!("{}", serde_json::to_string_pretty(&schema).unwrap()); return Ok(()); @@ -13384,7 +13390,7 @@ async fn main_inner() -> anyhow::Result<()> { // Build the clap Command and, when extensions are installed, append an // "EXTENSIONS:" section to the help output so they are visible in // `pup --help` / `pup help`, similar to how `gh` lists extensions. - let mut cmd = Cli::command(); + let mut cmd = cli_command(); #[cfg(not(target_arch = "wasm32"))] { let ext_help = extensions::discovery::build_extensions_help_section(); diff --git a/src/test_commands.rs b/src/test_commands.rs index 683dfcce..bbc6f2ff 100644 --- a/src/test_commands.rs +++ b/src/test_commands.rs @@ -461,19 +461,87 @@ fn test_extension_list_remote_parses() { } } +fn visible_top_level_command_names(app: &clap::Command) -> Vec { + app.get_subcommands() + .filter(|command| command.get_name() != "help" && !command.is_hide_set()) + .map(|command| command.get_name().to_string()) + .collect() +} + +fn rendered_top_level_command_names(app: &mut clap::Command) -> Vec { + let visible_names = visible_top_level_command_names(app); + let help = app.render_help().to_string(); + + help.lines() + .skip_while(|line| line.trim() != "Commands:") + .skip(1) + .take_while(|line| !line.trim().is_empty()) + .filter_map(|line| line.strip_prefix(" ")) + .filter(|line| !line.starts_with(' ')) + .filter_map(|line| line.split_whitespace().next()) + .filter(|name| visible_names.iter().any(|visible| visible == name)) + .map(str::to_string) + .collect() +} + #[test] fn test_top_level_commands_sorted_alphabetically() { + let mut app = crate::cli_command(); + let mut expected = visible_top_level_command_names(&app); + expected.sort_unstable(); + let names = rendered_top_level_command_names(&mut app); + + assert_eq!( + names, expected, + "top-level commands in help must be in alphabetical order.\nActual: {names:?}\nExpected: {expected:?}" + ); +} + +#[test] +fn test_top_level_commands_share_display_order() { + assert!( + crate::cli_command() + .get_subcommands() + .all(|command| command.get_display_order() == 0), + "top-level commands must share a display order so clap sorts them by name" + ); +} + +#[test] +fn test_top_level_command_names_and_aliases_are_unique() { let app = crate::Cli::command(); - let names: Vec<&str> = app - .get_subcommands() - .filter(|cmd| cmd.get_name() != "help" && !cmd.is_hide_set()) - .map(|cmd| cmd.get_name()) + let mut names = std::collections::HashMap::new(); + + for command in app.get_subcommands() { + for name in std::iter::once(command.get_name()).chain(command.get_all_aliases()) { + if let Some(existing) = names.insert(name, command.get_name()) { + panic!( + "top-level command name or alias `{name}` for `{}` conflicts with `{existing}`", + command.get_name() + ); + } + } + } +} + +#[test] +fn test_shared_display_order_sorts_appended_subcommand_in_help() { + let mut app = + crate::cli_command().subcommand(clap::Command::new("downtime-z-test").display_order(0)); + let names: Vec = rendered_top_level_command_names(&mut app) + .into_iter() + .filter(|name| { + matches!( + name.as_str(), + "downtime" | "downtime-z-test" | "error-tracking" + ) + }) .collect(); - let mut sorted = names.clone(); - sorted.sort_unstable(); + assert_eq!( - names, sorted, - "top-level commands must be in alphabetical order.\nActual: {names:?}\nExpected: {sorted:?}" + names, + ["downtime", "downtime-z-test", "error-tracking"], + "display order did not interleave the appended command" ); }