diff --git a/docs/src/guide/getting-started.md b/docs/src/guide/getting-started.md index 9ad1cbc8..6636302a 100644 --- a/docs/src/guide/getting-started.md +++ b/docs/src/guide/getting-started.md @@ -8,6 +8,12 @@ This guide walks through a basic fgumi workflow from FASTQ files to filtered con - A reference genome FASTA (with BWA index) - Paired-end FASTQ files with UMI sequences +## Boolean flags + +Flags that help renders as `[]` take an optional value. Given with no value they mean `true`, so `--write-index` and `--write-index true` are the same. They accept `true`, `t`, `yes`, `y`, `on`, and `1` for true; `false`, `f`, `no`, `n`, `off`, and `0` for false; and any capitalization of those. Anything else is rejected. + +The placeholder names only `true|false` because spelling out all twelve in every help line would be unreadable — the other spellings are accepted but not advertised per flag. + ## Basic Workflow ### 1. Extract UMIs from FASTQ diff --git a/src/lib/commands/sort.rs b/src/lib/commands/sort.rs index a24f3b20..dc0f3312 100644 --- a/src/lib/commands/sort.rs +++ b/src/lib/commands/sort.rs @@ -859,45 +859,9 @@ mod tests { } } - /// Whether `arg` takes an optional value, the shape every - /// `BoolishValueParser` flag on `Sort` uses (`num_args = 0..=1`, so a bare - /// `--flag` means `true`). - fn takes_optional_value(arg: &clap::Arg) -> bool { - arg.get_num_args().is_some_and(|range| range.min_values() == 0 && range.max_values() == 1) - } - - /// Boolean flags must name their accepted values, because clap otherwise - /// derives the placeholder from the field name: `--memory-per-thread` renders - /// as `--memory-per-thread []`, which reads as a request - /// for a per-thread memory size and invites `--memory-per-thread 10G`. - /// - /// `Sort` is `#[command(flatten)]`-ed into other binaries, where short `-h` - /// output is often all a user sees, so the placeholder has to carry this on - /// its own. - #[test] - fn test_bool_args_name_their_accepted_values() { - let command = Sort::command(); - - // Guard against a vacuous pass: if the `num_args` probe stops matching, - // the loop below would check nothing. - let bool_args: Vec<_> = - command.get_arguments().filter(|arg| takes_optional_value(arg)).collect(); - let matched = bool_args.len(); - assert!(matched >= 3, "expected Sort to expose its boolean flags, matched only {matched}"); - - for arg in bool_args { - let flag = arg.get_id(); - let value_names = arg - .get_value_names() - .map(|names| names.iter().map(ToString::to_string).collect::>()); - assert_eq!( - value_names.as_deref(), - Some(["true|false".to_string()].as_slice()), - "`--{flag}` must declare `value_name = \"true|false\"`; without it clap renders \ - the upper-cased field name, which reads as a value to supply rather than a bool", - ); - } - } + // Boolean-flag help is guarded repo-wide in `src/main.rs`'s `mod tests`, + // which walks every command rather than `Sort` alone and identifies boolean + // flags by their parser's accepted set rather than by `num_args`. // ======================================================================== // Temp-dir resolution tests diff --git a/src/main.rs b/src/main.rs index 6dd1800b..dabfd82f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -69,8 +69,17 @@ static GLOBAL: dhat::Alloc = dhat::Alloc; #[global_allocator] static GLOBAL: mimalloc::MiMalloc = mimalloc::MiMalloc; +/// Names the full set of spellings boolean flags accept. +/// +/// Per-flag help advertises only `[]`: `BoolishValueParser` takes +/// twelve literals, and naming all of them on every boolean flag would be +/// unreadable, so the complete set is stated once here instead. +const BOOL_FLAG_HELP: &str = "Boolean flags shown as [] take an optional value, and \ + mean true when given with none. They accept true, t, yes, y, on, 1 \ + and false, f, no, n, off, 0, in any capitalization."; + #[derive(Parser, Debug)] -#[command(styles = STYLES)] +#[command(styles = STYLES, after_help = BOOL_FLAG_HELP)] struct Args { /// Enable verbose (debug-level) logging. Equivalent to setting `RUST_LOG=debug`. #[arg(short, long, global = true)] @@ -219,3 +228,243 @@ fn main() -> Result<()> { result } + +#[cfg(test)] +mod tests { + use super::Args; + use clap::CommandFactory; + + /// Every literal `BoolishValueParser` accepts, lower-cased. + /// + /// clap marks ten of the twelve hidden, but `Arg::get_possible_values` + /// returns all of them — which is what lets `is_boolish` decide exactly + /// whether a flag is a boolean instead of guessing from its shape. + const BOOLISH_LITERALS: [&str; 12] = + ["true", "t", "yes", "y", "on", "1", "false", "f", "no", "n", "off", "0"]; + + /// The placeholder every boolean flag must render. + const BOOL_VALUE_NAME: &str = "true|false"; + + /// Commands the walk must reach, so a refactor that stops recursing into + /// subcommands fails loudly instead of passing vacuously. + const EXPECTED_COMMANDS: [&str; 6] = ["sort", "filter", "group", "dedup", "clip", "correct"]; + + /// A boolean flag located during the walk, carrying everything the + /// declaration assertions need so they never re-walk the tree. + /// + /// `command` is a path rather than the `clap::Command` itself because the + /// two tests that need one — the rendered-help and bare-parse tests — + /// mutate their copy, so they resolve a fresh one through + /// [`resolve_command`]. + struct BoolFlag { + /// Full command path, e.g. `fgumi compare bams`. + command: String, + /// The flag's id, e.g. `memory_per_thread`. + id: String, + /// The flag's long spelling, e.g. `memory-per-thread`. + long: Option, + /// `value_name`s the flag declares, if any. + value_names: Option>, + /// Whether the flag suppresses clap's `[possible values: ...]` line. + hides_possible_values: bool, + /// Whether the flag is hidden from help, and so renders no placeholder. + hidden: bool, + } + + /// Whether `arg` is parsed by `BoolishValueParser`. + /// + /// Keyed on the parser's own accepted set, not on a structural proxy such as + /// `num_args = 0..=1`: an optional-value flag that is not a boolean — an + /// optional path, an optional count — has that same shape, and holding it to + /// the boolean placeholder would be wrong. + fn is_boolish(arg: &clap::Arg) -> bool { + let accepted: Vec = + arg.get_possible_values().iter().map(|v| v.get_name().to_ascii_lowercase()).collect(); + accepted.len() == BOOLISH_LITERALS.len() + && accepted.iter().all(|value| BOOLISH_LITERALS.contains(&value.as_str())) + } + + /// Collect every boolean flag in the CLI, recursing through subcommands so + /// nested commands (`compare bams`, `simulate fastq`) are covered too. + fn collect_bool_flags(command: &clap::Command, path: &str, out: &mut Vec) { + for arg in command.get_arguments().filter(|arg| is_boolish(arg)) { + out.push(BoolFlag { + command: path.to_string(), + id: arg.get_id().to_string(), + long: arg.get_long().map(ToString::to_string), + value_names: arg + .get_value_names() + .map(|names| names.iter().map(ToString::to_string).collect()), + hides_possible_values: arg.is_hide_possible_values_set(), + hidden: arg.is_hide_set(), + }); + } + for sub in command.get_subcommands() { + collect_bool_flags(sub, &format!("{path} {}", sub.get_name()), out); + } + } + + /// Re-resolve a `BoolFlag::command` path back to the `clap::Command` it + /// names, e.g. `fgumi compare bams` to the `bams` subcommand. + fn resolve_command(path: &str) -> clap::Command { + let mut command = Args::command(); + for name in path.split_whitespace().skip(1) { + let sub = command + .get_subcommands() + .find(|sub| sub.get_name() == name) + .unwrap_or_else(|| panic!("no subcommand `{name}` under `{path}`")) + .clone(); + command = sub; + } + command + } + + /// The long spelling of `flag`, which every boolean flag is required to + /// have: a bare boolean is only usable as `--flag`, and a short-only + /// boolean could not render the placeholder this module asserts. + fn long_of(flag: &BoolFlag) -> &str { + flag.long.as_deref().unwrap_or_else(|| { + panic!("`{} --{}` is a boolean flag with no long spelling", flag.command, flag.id) + }) + } + + /// Walk the CLI once and assert it was actually walked. + fn bool_flags() -> Vec { + let mut flags = Vec::new(); + collect_bool_flags(&Args::command(), "fgumi", &mut flags); + + // Guard against a vacuous pass: if `is_boolish` or the subcommand + // recursion stops matching, every loop over this would check nothing. + assert!( + flags.len() >= 40, + "expected the CLI to expose its boolean flags, got {}", + flags.len() + ); + for expected in EXPECTED_COMMANDS { + assert!( + flags.iter().any(|flag| flag.command.ends_with(expected)), + "walk never reached `{expected}`", + ); + } + flags + } + + /// Every boolean flag must declare `value_name = "true|false"`. + /// + /// Without it clap derives the placeholder from the field name, so + /// `--memory-per-thread` renders as `--memory-per-thread + /// []` — which reads as a request for a per-thread memory + /// size and invites `--memory-per-thread 10G`. `QueueMemoryOptions` is + /// `#[command(flatten)]`-ed into most commands, so the placeholder has to + /// carry this on its own in each of them. + #[test] + fn test_bool_args_name_their_accepted_values() { + for flag in bool_flags() { + assert_eq!( + flag.value_names.as_deref(), + Some([BOOL_VALUE_NAME.to_string()].as_slice()), + "`{} --{}` must declare `value_name = \"{BOOL_VALUE_NAME}\"`; without it clap \ + renders the upper-cased field name, which reads as a value to supply rather \ + than a bool", + flag.command, + flag.id, + ); + } + } + + /// Every boolean flag must suppress clap's `[possible values: ...]` line. + /// + /// `BoolishValueParser` hides ten of its twelve literals, so clap would + /// advertise `true, false` alone — naming two of the twelve accepted + /// spellings is worse than naming none, which is why the placeholder carries + /// the contract instead. + #[test] + fn test_bool_args_hide_their_partial_possible_values() { + for flag in bool_flags() { + assert!( + flag.hides_possible_values, + "`{} --{}` must set `hide_possible_values = true`; clap hides ten of the twelve \ + boolish literals, so help would advertise only `true, false`", + flag.command, flag.id, + ); + } + } + + /// Since per-flag help advertises only two of the twelve accepted + /// spellings, the top-level help has to name the rest — and has to keep + /// naming them if the parser's set ever changes. + #[test] + fn test_top_level_help_names_every_accepted_spelling() { + let help = super::BOOL_FLAG_HELP.to_ascii_lowercase(); + for literal in BOOLISH_LITERALS { + assert!( + help.split(|c: char| !c.is_ascii_alphanumeric()).any(|word| word == literal), + "top-level help does not name the accepted spelling `{literal}`; help was: {help}", + ); + } + assert!( + Args::command().render_help().to_string().contains("Boolean flags shown as"), + "`fgumi --help` does not carry the boolean-flag note", + ); + } + + /// The declared `value_name` is only half the contract — assert what a user + /// actually sees, for every flag rather than once per command. + /// + /// The rendered placeholder also depends on `num_args`: `0..=1` renders + /// `[]`, while a required `1` renders `` and would + /// silently break bare `--verify` while still satisfying the declaration + /// test above. Asserting per flag rather than per command matters because + /// most commands own several booleans, and a command-level assertion passes + /// as long as *one* of them renders the placeholder. Flags marked + /// `hide = true` are exempt: they render nothing at all, so there is no + /// placeholder to hold them to — the declaration tests above still cover + /// them, as does the bare-parse test below. + #[test] + fn test_bool_args_render_an_optional_boolean_placeholder() { + let mut checked = 0_usize; + for flag in bool_flags().into_iter().filter(|flag| !flag.hidden) { + checked += 1; + let help = resolve_command(&flag.command).render_help().to_string(); + let expected = format!("--{} [<{BOOL_VALUE_NAME}>]", long_of(&flag)); + assert!( + help.contains(&expected), + "`{} --help` renders no `{expected}`; a boolean flag must render its value as \ + optional, since a required `<{BOOL_VALUE_NAME}>` breaks the bare `--{}` form; \ + help was:\n{help}", + flag.command, + long_of(&flag), + ); + } + // Guard against a vacuous pass: if every boolean flag were hidden, or + // the walk stopped finding them, the loop above would assert nothing. + assert!(checked >= 40, "expected the CLI to expose visible boolean flags, got {checked}"); + } + + /// Passing a boolean flag with no value must mean `true`. + /// + /// `num_args = 0..=1` alone only makes the value optional — without + /// `default_missing_value = "true"` clap errors on the bare form, so the + /// documented "mean true when given with none" contract needs a parse to + /// hold it. Required arguments are relaxed first so each command can be + /// probed with its boolean flag alone. + #[test] + fn test_bool_args_parse_their_bare_long_option_as_true() { + for flag in bool_flags() { + let long = long_of(&flag).to_string(); + let command = resolve_command(&flag.command).mut_args(|arg| arg.required(false)); + let matches = command + .try_get_matches_from(["probe", &format!("--{long}")]) + .unwrap_or_else(|error| { + panic!("`{} --{long}` (no value) failed to parse: {error}", flag.command) + }); + assert_eq!( + matches.get_one::(&flag.id), + Some(&true), + "`{} --{long}` (no value) must parse as `true`; set \ + `default_missing_value = \"true\"`", + flag.command, + ); + } + } +}