From b16ace6d72cec1a8d5956421eb20148de0384257 Mon Sep 17 00:00:00 2001 From: Nils Homer Date: Tue, 28 Jul 2026 16:28:33 -0700 Subject: [PATCH] docs(sort): keep the binary name out of per-argument help `Sort` is pulled into other binaries with `#[command(flatten)]`, so its per-argument help renders under a program name that is not `fgumi`. The `--sort-threads` help embedded a worked `bwa mem -t 32 ... | fgumi sort -@ 8 --sort-threads 4` pipeline, which tells those users to run a command they do not have. It was also the only per-argument doc carrying a concrete invocation; every other one already lives in the command-level EXAMPLES block. Move the pipeline to EXAMPLES and leave the flag help describing the flag. The replacement also states why the merge can stay wide -- it cannot start until the input is exhausted, by which point the producer has finished writing -- which the previous "the merge does not" left implicit. `enter_output_phase` is called exactly once, immediately after ingest completes, so that holds for every sort path. `--order` had a milder version of the same thing ("fgumi emits `queryname:lexicographical` in @HD SS"); it is now written passively, which stays true of any binary embedding the engine. A new test walks every argument's help and rejects the binary name so this cannot creep back. The match is on identifier-shaped tokens rather than the literal `"fgumi "`, so a trailing, backticked, or punctuated mention is caught too, while `fgumidocs` and `fgumi_sort` are not. Command-level `long_about` is deliberately exempt and is not walked: a wrapper replaces it wholesale, so it remains the right home for worked `fgumi sort` invocations. --- src/lib/commands/sort.rs | 75 +++++++++++++++++++++++++++++++++++++--- 1 file changed, 70 insertions(+), 5 deletions(-) diff --git a/src/lib/commands/sort.rs b/src/lib/commands/sort.rs index 155c70d78..675bb61d8 100644 --- a/src/lib/commands/sort.rs +++ b/src/lib/commands/sort.rs @@ -180,6 +180,9 @@ EXAMPLES: # Reserve extra memory for bwa mem running in a pipeline fgumi sort -i input.bam -o sorted.bam --memory-reserve 12GiB --threads 4 + # Cede cores to the aligner during ingest, but keep the merge wide + bwa mem -t 32 ref.fa r1.fq r2.fq | fgumi sort -i - -o sorted.bam -@ 8 --sort-threads 4 + # Allow more spilled runs before consolidating (fewer consolidation passes) fgumi sort -i input.bam -o sorted.bam --order coordinate --max-temp-files 512 @@ -216,7 +219,7 @@ pub struct Sort { /// Queryname sort supports sub-sort specifiers: /// `queryname` Lexicographic byte ordering (default, fast) /// `queryname::lexicographic` Explicit lexicographic ordering (alias: `queryname::lex`) - /// `queryname::lexicographical` Alias; fgumi emits `queryname:lexicographical` in `@HD` SS + /// `queryname::lexicographical` Alias; written as `queryname:lexicographical` in `@HD` SS /// `queryname::natural` Natural numeric ordering (samtools-compatible) #[arg(long = "order", default_value = "template-coordinate", value_parser = SortOrderArg::parse)] pub order: SortOrderArg, @@ -291,9 +294,10 @@ pub struct Sort { /// Number of threads for the sort phase (accumulate, sort, spill). /// /// Defaults to `--threads`. Lower this to cede cores to an upstream - /// producer while keeping the merge wide -- e.g. in - /// `bwa mem -t 32 ... | fgumi sort -@ 8 --sort-threads 4`, ingest competes - /// with the aligner but the merge does not. + /// producer while keeping the merge wide -- with `-@ 8 --sort-threads 4`, + /// ingest contends with the producer over only 4 threads, while the merge + /// still uses 8 because it cannot start until the input is exhausted, by + /// which point the producer has finished writing. /// /// This only changes scheduling; the output is byte-identical. #[arg(long = "sort-threads")] @@ -703,9 +707,70 @@ mod tests { // Memory-budget helpers moved to `commands::common`; import the `pub(crate)` // items these tests exercise that are not re-exported through `super::*`. use crate::commands::common::{MIN_MEMORY_PER_THREAD, detect_total_memory, resolve_reserve}; - use clap::Parser; + use clap::{CommandFactory, Parser}; use rstest::rstest; + // ======================================================================== + // Help-text tests + // ======================================================================== + + /// Returns true if `help` names the `fgumi` binary as a standalone token. + /// + /// Splits on every character that cannot appear inside a Rust identifier, so + /// a bare `fgumi`, a trailing `fgumi`, and punctuated forms (`` `fgumi` ``, + /// `fgumi.`, `fgumi-sort`) all match, while `fgumi` embedded in a longer word + /// (`fgumidocs`) and identifier-shaped mentions (`fgumi_sort`) do not. + fn names_the_binary(help: &str) -> bool { + help.split(|c: char| !c.is_ascii_alphanumeric() && c != '_').any(|token| token == "fgumi") + } + + #[rstest] + #[case::bare_invocation("run fgumi sort to order records", true)] + #[case::trailing_token("this flag mirrors fgumi", true)] + #[case::followed_by_period("see fgumi.", true)] + #[case::backticked("see `fgumi` for details", true)] + #[case::hyphenated("see fgumi-sort", true)] + #[case::embedded_in_word("see the fgumidocs site", false)] + #[case::suffix_of_word("see myfgumi", false)] + #[case::rust_identifier("handled by fgumi_sort::run", false)] + #[case::no_mention("Sort records by template coordinate", false)] + fn test_names_the_binary(#[case] help: &str, #[case] expected: bool) { + assert_eq!(names_the_binary(help), expected, "help was: {help}"); + } + + /// `Sort` is `#[command(flatten)]`-ed into other binaries, so its + /// per-argument help renders under a program name that is not `fgumi` — a + /// concrete `fgumi ...` invocation there tells those users to run a command + /// they do not have. Per-argument help must therefore describe the flag + /// without naming the binary. + /// + /// The command-level `long_about` is deliberately exempt, and is not walked + /// here: a wrapper replaces it wholesale, so its EXAMPLES block is the right + /// home for worked `fgumi sort` invocations. + #[test] + fn test_arg_help_does_not_name_the_binary() { + let command = Sort::command(); + + // Guard against a vacuous pass: an empty (or trivially short) arg walk + // would satisfy the loop below without checking anything. + let arg_count = command.get_arguments().count(); + assert!(arg_count > 10, "expected Sort to expose its flags, walked only {arg_count}"); + + for arg in command.get_arguments() { + let help = format!( + "{} {}", + arg.get_help().map(ToString::to_string).unwrap_or_default(), + arg.get_long_help().map(ToString::to_string).unwrap_or_default(), + ); + assert!( + !names_the_binary(&help), + "help for `--{}` names the `fgumi` binary; describe the flag instead and put \ + worked invocations in the command-level EXAMPLES block. Help was: {help}", + arg.get_id() + ); + } + } + // ======================================================================== // Temp-dir resolution tests // ========================================================================