Repository navigation
docs(sort): name the accepted values for sort's boolean flags - #690
Merged
Merged
Conversation
Without an explicit `value_name`, clap derives an option's value placeholder from the field name, so `--memory-per-thread` rendered as `--memory-per-thread [<MEMORY_PER_THREAD>]`. That reads as a request for a per-thread memory size rather than a toggle, and invites `--memory-per-thread 10G`. Short `-h` output compounds it: it shows only the first line of the doc comment, which does not say the flag takes a boolean. Declare `value_name = "true|false"` on the three optional-value booleans -- `--verify`, `--memory-per-thread`, and `--write-index` -- so the placeholder names what `parse_bool` accepts. `Sort` is `#[command(flatten)]`-ed into other binaries, where the placeholder is often all a user sees, so it has to carry this on its own. Help text only: parsing is unchanged, and the existing error message already enumerates the accepted values.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe Sort command now displays ChangesSort boolean option values
Estimated code review effort: 2 (Simple) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Member
|
@coderabbitai pause |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/commands/sort.rs`:
- Around line 774-800: The test test_bool_args_name_their_accepted_values
currently identifies arguments only by optional-value arity, allowing unrelated
arguments to be included and expected boolean flags to be omitted. Define the
expected IDs for verify, memory-per-thread, and write-index, assert that the
filtered arguments match exactly those IDs, and restrict the "true / false"
value_name checks to that explicit set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0539718d-ed67-4927-9602-db1de056c00a
📒 Files selected for processing (1)
src/lib/commands/sort.rs
Comment on lines
+774
to
+800
| /// Whether `arg` takes an optional value, the shape every `parse_bool` 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 [<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(); | ||
| assert!( | ||
| bool_args.len() >= 3, | ||
| "expected Sort to expose its boolean flags, matched only {}", | ||
| bool_args.len() | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate Cargo.toml / rust-toolchain =="
fd -a '^(Cargo\.toml|rust-toolchain\.toml)$' . | sed 's#^\./##'
echo
echo "== relevant sort test lines =="
sed -n '740,830p' src/lib/commands/sort.rs
echo
echo "== clap dependency declarations =="
rg -n 'name = "clap"|clap\s*=' Cargo.toml Cargo.lock --glob 'Cargo.toml' --glob 'Cargo.lock' | head -120 || true
echo
echo "== Sort argument definitions around claimed IDs =="
rg -n -C 3 'verify|milk-per-thread|memory-per-thread|write-index' src/lib/commands/sort.rs || true
echo
echo "== rust-version/toolchain =="
rg -n 'rust-version|channel|build' Cargo.toml rust-toolchain.toml || trueRepository: fulcrumgenomics/fgumi
Length of output: 1931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate Cargo.toml / rust-toolchain =="
fd -a '^(Cargo\.toml|rust-toolchain\.toml)$' . | sed 's#^\./##'
echo
echo "== relevant sort test lines =="
sed -n '740,830p' src/lib/commands/sort.rs
echo
echo "== clap dependency declarations =="
rg -n 'name = "clap"|clap\s*=' Cargo.toml Cargo.lock --glob 'Cargo.toml' --glob 'Cargo.lock' | head -120 || true
echo
echo "== Sort argument definitions around claimed IDs =="
rg -n -C 3 'verify|milk-per-thread|memory-per-thread|write-index' src/lib/commands/sort.rs || true
echo
echo "== rust-version/toolchain =="
rg -n 'rust-version|channel|build' Cargo.toml rust-toolchain.toml || trueRepository: fulcrumgenomics/fgumi
Length of output: 1931
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repository filesystem =="
pwd
git ls-files | head -50
echo
echo "== locate Cargo.toml / rust-toolchain =="
fd -a '^(Cargo\.toml|rust-toolchain\.toml)$' . | sed 's#^\./##'
echo
echo "== sort.rs argument tests and definitions =="
ast-grep outline src/lib/commands/sort.rs --view compact || true
sed -n '740,830p' src/lib/commands/sort.rs
rg -n -C 3 'verify|milk-per-thread|memory-per-thread|write-index' src/lib/commands/sort.rs || true
echo
echo "== clap dependency declarations =="
rg -n 'name = "clap"|clap\s*=' Cargo.toml Cargo.lock --glob 'Cargo.toml' --glob 'Cargo.lock' | head -120 || true
echo
echo "== rust-version/toolchain =="
rg -n 'rust-version|channel|build' Cargo.toml rust-toolchain.toml || trueRepository: fulcrumgenomics/fgumi
Length of output: 1931
🌐 Web query:
fulcrumgenomics fgumi src/lib/commands/sort.rs test_bool_args_name_their_accepted_values verify memory-per-thread write-index
💡 Result:
In the context of the fulcrumgenomics/fgumi repository, the components you referenced relate to the following technical details: Memory-per-thread and Sorting The fgumi tool suite, specifically the sort command, uses a memory model that differs from legacy Java-based tools like fgbio [1]. In fgumi, options such as memory-per-thread and queue-memory settings are used to manage backpressure and RAM usage [1]. Documentation highlights that the --queue-memory setting is per-thread by default, and users are advised to use --max-memory when running fgumi sort to effectively limit overall RAM usage [1]. Repository Tests The reference to test_sort_write_index is associated with recent updates in the fgumi repository, specifically regarding the addition of comprehensive pipeline concurrency tests [2]. This test ensures the correctness of operations within the tool's sorting and pipeline infrastructure [2]. The query test_bool_args_name_their_accepted_values does not appear in the official public fgumi repository documentation or source code based on available information; it is likely a generic or external test case unrelated to the fgumi codebase [3][4][5].
Citations:
- 1: https://github.com/fulcrumgenomics/fgumi/blob/main/docs/src/guide/best-practices.md
- 2: 320ad4c
- 3: https://github.com/NousResearch/hermes-agent/blob/72ff3e90/tests/run_agent/test_tool_arg_coercion.py
- 4: https://github.com/ansible/ansible/blob/0e69d86c/test/units/module_utils/common/arg_spec/test_validate_valid.py
- 5: https://github.com/python/cpython/blob/main/Lib/test/test_argparse.py
🌐 Web query:
"test_bool_args_name_their_accepted_values" "fn takes_optional_value" "src/lib/commands/sort.rs"
💡 Result:
Based on the source code of the Nushell project (nushell/nushell), the function name test_bool_args_name_their_accepted_values does not exist within the file src/lib/commands/sort.rs or the broader codebase. The Nushell codebase does contain a sort command defined in crates/nu-command/src/filters/sort.rs [1][2], and its associated tests in crates/nu-command/tests/commands/sort.rs [3][4]. These files handle command-line flags for the sort command (such as --reverse, --ignore-case, --natural, and --values) using standard Nushell command-parsing patterns [1][2], but they do not contain a function with that specific name. The query "takes_optional_value" is a common concept in command-line argument parsing libraries, particularly in Rust's clap crate [5][6][7], but it is not a specific function found in the referenced Nushell sort command source file. It is likely that the requested function name is either a hypothetical test case, a misremembered name, or specific to a different project or context.
Citations:
- 1: https://github.com/nushell/nushell/blob/90d82b7c/crates/nu-command/src/filters/sort.rs
- 2: https://github.com/nushell/nushell/blob/4c0c6ca6/crates/nu-command/src/filters/sort.rs
- 3: https://github.com/nushell/nushell/blob/4c0c6ca6/crates/nu-command/tests/commands/sort.rs
- 4: https://github.com/nushell/nushell/blob/90d82b7c/crates/nu-command/tests/commands/sort.rs
- 5: Flag takes optional value clap-rs/clap#1354
- 6: takes_value does not always works clap-rs/clap#984
- 7: optional
takes_valueclap-rs/clap#367
🌐 Web query:
clap derive value_name bool flag 0..=1 parse_bool default_missing_value value_name true false
💡 Result:
In the clap derive API, a field of type bool is treated as a flag by default [1][2], which automatically uses ArgAction::SetTrue [1][3]. Because this behavior is specialized, it does not natively support passing explicit "true" or "false" values (e.g., --flag=false) [4][5]. To support explicit parsing of true/false values (e.g., --flag=true or --flag=false) while keeping the flag behavior, you must override the default action and configuration [4][6]. The common pattern involves setting num_args to 0..=1, overriding the action to ArgAction::Set, and providing default values [4][6]. Example configuration for manual boolean parsing: #[arg( long, num_args(0..=1), default_value("false"), default_missing_value("true"), action = clap::ArgAction::Set)] pub flag: bool, In this configuration: - num_args(0..=1) allows the flag to be used without an argument (e.g., --flag) or with an argument (e.g., --flag=false) [4][6]. - default_missing_value("true") ensures that if the user provides the flag without a value, it evaluates to true [4][6]. - default_value("false") sets the initial state if the flag is omitted entirely [4][6]. - action = ArgAction::Set instructs clap to treat the input as a value rather than a simple switch [6]. Note that value_name is used to change the placeholder text in the generated help message (e.g., #[arg(long, value_name = "BOOL")]) [7][8]. If you require a boolean that defaults to true and can be set to false, you can use similar logic by changing the default_value to "true" and using ArgAction::SetFalse, or by adjusting the default values accordingly [9].
Citations:
- 1: https://docs.rs/clap/latest/clap/_derive/
- 2: https://github.com/clap-rs/clap/blob/master/src/_derive/mod.rs
- 3: https://docs.rs/clap/latest/clap/_derive/_tutorial/chapter_2/index.html
- 4: the default derive for booleans should also handle explicit
--flag=true,--flag=falseclap-rs/clap#5577 - 5: https://stackoverflow.com/questions/77771008/how-do-i-create-a-rust-clap-derive-boolean-flag-that-is-defaulted-to-true-and-ca
- 6: Allow boolean literals as values for flags clap-rs/clap#1649
- 7:
nameimplies a user-facing meaning, causing confusion clap-rs/clap#3335 - 8: Different help text for bool and Option<bool> clap-rs/clap#5694
- 9: https://users.rust-lang.org/t/boolean-arguments-in-clap/125508
Assert the expected boolean argument IDs explicitly.
This test filters by optional-value num_args only, so non-boolean optional arguments are included and a missing verify, memory-per-thread, or write-index flag is only reported when fewer than three optional-value arguments remain. List the expected IDs and assert that only those IDs are checked for "true" / "false" value_name.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/lib/commands/sort.rs` around lines 774 - 800, The test
test_bool_args_name_their_accepted_values currently identifies arguments only by
optional-value arity, allowing unrelated arguments to be included and expected
boolean flags to be omitted. Define the expected IDs for verify,
memory-per-thread, and write-index, assert that the filtered arguments match
exactly those IDs, and restrict the "true / false" value_name checks to that
explicit set.
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #690 +/- ##
=======================================
Coverage 93.95% 93.95%
=======================================
Files 178 178
Lines 108059 108077 +18
=======================================
+ Hits 101524 101542 +18
Misses 6535 6535 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
`codecov/patch` failed the PR at 89.47% against a 90% target. The two lines it counted as missed were the `assert!`/`assert_eq!` failure-message arguments (`bool_args.len()` and `arg.get_id()`), which only evaluate when an assertion fires and so never run in a passing suite. Bind both to locals and interpolate them inline, so every line in the test executes on the passing path and the messages read the same. This is the form clippy's `uninlined_format_args` prefers anyway; it was only inapplicable while the arguments were call expressions.
Merged
nh13
added a commit
that referenced
this pull request
Aug 1, 2026
Replace the hand-rolled `parse_bool` with clap's `BoolishValueParser` at all 44 boolean `#[arg]` sites, and delete the function. The accepted set is a strict superset: `on`, `off`, `1`, and `0` join the existing `true`/`false`/`yes`/`no`/`y`/`n`/`t`/`f`, all case-insensitive. Nothing that parsed before parses differently now. Unrecognized input is still rejected -- the parser returns an error rather than falling back to `true`, which clap's own `str_to_bool` doc comment misleadingly suggests. `parse_bool` documented itself as matching sopt/fgbio, and widening past that set is the one real consequence here. It is deliberate: the flags gain the spellings users coming from other CLIs reach for first, and both directions of the change are pinned by tests. Each site also gains `hide_possible_values = true` and `value_name = "true|false"`, which are one decision rather than two. Without the first, clap advertises `[possible values: true, false]`, because it hides the other ten literals -- help that names two of the twelve accepted spellings is worse than help that names none. But suppressing that line makes the placeholder the only in-help signal about what the flag takes, and clap derives that placeholder from the field name: `--memory-per-thread [<MEMORY_PER_THREAD>]` reads as a request for a per-thread memory size and invites `--memory-per-thread 10G`. #690 fixed the placeholder for the three flags on `Sort`; the other 41 were left deriving it, so the same flag rendered two ways depending on which command you asked -- `QueueMemoryOptions::memory_per_thread` is flattened into dedup, group, filter, clip, correct, and the consensus callers. `--allow-unmapped` was worse, setting `ALLOW_UNMAPPED` explicitly. All 44 now render `[<true|false>]`. The two rstest tables that covered `parse_bool` directly now drive the same cases through `TestBoolFlags::try_parse_from`, so they assert the CLI contract rather than a private function and survive another change of parser. `on`/`off`/`1`/`0` moved from the rejected table to the accepted one -- that migration is the behaviour change, made explicit. `test_bool_args_name_their_accepted_values` (from #690) still guards `Sort` alone and identifies boolean args by the `num_args = 0..=1` proxy rather than by their parser; both are addressed in a follow-up, now that `Arg::get_possible_values()` surfaces the boolish literal set. Note `crates/fgumi-cli-common` (on main-runall, not here) carries a second copy of `parse_bool` per the landing tracker's duplication ledger. It must not be reinstated as the live parser when the umbrella points at that crate. cargo ci-fmt/ci-lint clean; 6891 tests pass, 27 skipped. Help rendering verified on the built binary across sort, filter, dedup, group, clip, correct, and simplex.
nh13
added a commit
that referenced
this pull request
Aug 2, 2026
Replace the hand-rolled `parse_bool` with clap's `BoolishValueParser` at all 44 boolean `#[arg]` sites, and delete the function. The accepted set is a strict superset: `on`, `off`, `1`, and `0` join the existing `true`/`false`/`yes`/`no`/`y`/`n`/`t`/`f`, all case-insensitive. Nothing that parsed before parses differently now. Unrecognized input is still rejected -- the parser returns an error rather than falling back to `true`, which clap's own `str_to_bool` doc comment misleadingly suggests. `parse_bool` documented itself as matching sopt/fgbio, and widening past that set is the one real consequence here. It is deliberate: the flags gain the spellings users coming from other CLIs reach for first, and both directions of the change are pinned by tests. Each site also gains `hide_possible_values = true` and `value_name = "true|false"`, which are one decision rather than two. Without the first, clap advertises `[possible values: true, false]`, because it hides the other ten literals -- help that names two of the twelve accepted spellings is worse than help that names none. But suppressing that line makes the placeholder the only in-help signal about what the flag takes, and clap derives that placeholder from the field name: `--memory-per-thread [<MEMORY_PER_THREAD>]` reads as a request for a per-thread memory size and invites `--memory-per-thread 10G`. were left deriving it, so the same flag rendered two ways depending on which command you asked -- `QueueMemoryOptions::memory_per_thread` is flattened into dedup, group, filter, clip, correct, and the consensus callers. `--allow-unmapped` was worse, setting `ALLOW_UNMAPPED` explicitly. All 44 now render `[<true|false>]`. The two rstest tables that covered `parse_bool` directly now drive the same cases through `TestBoolFlags::try_parse_from`, so they assert the CLI contract rather than a private function and survive another change of parser. `on`/`off`/`1`/`0` moved from the rejected table to the accepted one -- that migration is the behaviour change, made explicit. `test_bool_args_name_their_accepted_values` (from #690) still guards `Sort` alone and identifies boolean args by the `num_args = 0..=1` proxy rather than by their parser; both are addressed in a follow-up, now that `Arg::get_possible_values()` surfaces the boolish literal set. Note `crates/fgumi-cli-common` (on main-runall, not here) carries a second copy of `parse_bool` per the landing tracker's duplication ledger. It must not be reinstated as the live parser when the umbrella points at that crate. cargo ci-fmt/ci-lint clean; 6891 tests pass, 27 skipped. Help rendering verified on the built binary across sort, filter, dedup, group, clip, correct, and simplex.
nh13
added a commit
that referenced
this pull request
Aug 2, 2026
#690 added `test_bool_args_name_their_accepted_values` to `sort.rs`. It guarded `Sort` alone, and it identified boolean flags by the structural proxy `num_args = 0..=1` -- a shape an optional path or optional count shares, so the first non-boolean optional-value flag added to `Sort` would have failed it with a message telling the author to declare `value_name = "true|false"` on it. There was no way to do better against `parse_bool`: clap keeps `default_missing_vals` private with no getter, and `ArgAction` does not implement `PartialEq`. `BoolishValueParser` changes that. `Arg::get_possible_values()` surfaces the parser's own twelve literals (ten of them hidden), so `is_boolish` now decides exactly whether a flag is a boolean rather than inferring it from shape. That precision is what makes a repo-wide walk safe, so the guard moves to `src/main.rs` and recurses from `Args::command()` through every subcommand, including nested ones. It covers 44 flags across 19 commands instead of 3 on one, and it cannot drift from the CLI, because it walks the CLI rather than a parallel list. Four tests, each asserting a distinct half of the contract: - `..._name_their_accepted_values` -- every boolean flag declares `value_name = "true|false"`. - `..._hide_their_partial_possible_values` -- every boolean flag sets `hide_possible_values`, so help never advertises `true, false` alone. - `..._render_an_optional_boolean_placeholder` -- the rendered help of every command owning a boolean flag actually contains `[<true|false>]`. The declaration test cannot catch this on its own: the placeholder also depends on `num_args`, so changing `0..=1` to a required `1` would break bare `--verify` while still declaring the right `value_name`. - `..._top_level_help_names_every_accepted_spelling` -- ties the new top-level note to `BOOLISH_LITERALS`, so a change to the parser's set that leaves the documentation behind fails. Both vacuity guards are load-bearing and were verified by mutation: stripping `value_name` from `QueueMemoryOptions::memory_per_thread` fails the declaration and render tests naming `fgumi extract --memory_per_thread`, and stripping `hide_possible_values` fails the third. Since per-flag help names two of the twelve accepted spellings, the other ten are documented once -- in `fgumi --help` via `after_help`, and in the getting-started guide -- rather than repeated in 44 doc comments, where they would drift. cargo ci-fmt/ci-lint clean; 6894 tests pass, 27 skipped.
nh13
added a commit
that referenced
this pull request
Aug 6, 2026
Replace the hand-rolled `parse_bool` with clap's `BoolishValueParser` at all 44 boolean `#[arg]` sites, and delete the function. The accepted set is a strict superset: `on`, `off`, `1`, and `0` join the existing `true`/`false`/`yes`/`no`/`y`/`n`/`t`/`f`, all case-insensitive. Nothing that parsed before parses differently now. Unrecognized input is still rejected -- the parser returns an error rather than falling back to `true`, which clap's own `str_to_bool` doc comment misleadingly suggests. `parse_bool` documented itself as matching sopt/fgbio, and widening past that set is the one real consequence here. It is deliberate: the flags gain the spellings users coming from other CLIs reach for first, and both directions of the change are pinned by tests. Each site also gains `hide_possible_values = true` and `value_name = "true|false"`, which are one decision rather than two. Without the first, clap advertises `[possible values: true, false]`, because it hides the other ten literals -- help that names two of the twelve accepted spellings is worse than help that names none. But suppressing that line makes the placeholder the only in-help signal about what the flag takes, and clap derives that placeholder from the field name: `--memory-per-thread [<MEMORY_PER_THREAD>]` reads as a request for a per-thread memory size and invites `--memory-per-thread 10G`. were left deriving it, so the same flag rendered two ways depending on which command you asked -- `QueueMemoryOptions::memory_per_thread` is flattened into dedup, group, filter, clip, correct, and the consensus callers. `--allow-unmapped` was worse, setting `ALLOW_UNMAPPED` explicitly. All 44 now render `[<true|false>]`. The two rstest tables that covered `parse_bool` directly now drive the same cases through `TestBoolFlags::try_parse_from`, so they assert the CLI contract rather than a private function and survive another change of parser. `on`/`off`/`1`/`0` moved from the rejected table to the accepted one -- that migration is the behaviour change, made explicit. `test_bool_args_name_their_accepted_values` (from #690) still guards `Sort` alone and identifies boolean args by the `num_args = 0..=1` proxy rather than by their parser; both are addressed in a follow-up, now that `Arg::get_possible_values()` surfaces the boolish literal set. Note `crates/fgumi-cli-common` (on main-runall, not here) carries a second copy of `parse_bool` per the landing tracker's duplication ledger. It must not be reinstated as the live parser when the umbrella points at that crate. cargo ci-fmt/ci-lint clean; 6891 tests pass, 27 skipped. Help rendering verified on the built binary across sort, filter, dedup, group, clip, correct, and simplex.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
…#703) Replace the hand-rolled `parse_bool` with clap's `BoolishValueParser` at all 44 boolean `#[arg]` sites, and delete the function. The accepted set is a strict superset: `on`, `off`, `1`, and `0` join the existing `true`/`false`/`yes`/`no`/`y`/`n`/`t`/`f`, all case-insensitive. Nothing that parsed before parses differently now. Unrecognized input is still rejected -- the parser returns an error rather than falling back to `true`, which clap's own `str_to_bool` doc comment misleadingly suggests. `parse_bool` documented itself as matching sopt/fgbio, and widening past that set is the one real consequence here. It is deliberate: the flags gain the spellings users coming from other CLIs reach for first, and both directions of the change are pinned by tests. Each site also gains `hide_possible_values = true` and `value_name = "true|false"`, which are one decision rather than two. Without the first, clap advertises `[possible values: true, false]`, because it hides the other ten literals -- help that names two of the twelve accepted spellings is worse than help that names none. But suppressing that line makes the placeholder the only in-help signal about what the flag takes, and clap derives that placeholder from the field name: `--memory-per-thread [<MEMORY_PER_THREAD>]` reads as a request for a per-thread memory size and invites `--memory-per-thread 10G`. were left deriving it, so the same flag rendered two ways depending on which command you asked -- `QueueMemoryOptions::memory_per_thread` is flattened into dedup, group, filter, clip, correct, and the consensus callers. `--allow-unmapped` was worse, setting `ALLOW_UNMAPPED` explicitly. All 44 now render `[<true|false>]`. The two rstest tables that covered `parse_bool` directly now drive the same cases through `TestBoolFlags::try_parse_from`, so they assert the CLI contract rather than a private function and survive another change of parser. `on`/`off`/`1`/`0` moved from the rejected table to the accepted one -- that migration is the behaviour change, made explicit. `test_bool_args_name_their_accepted_values` (from #690) still guards `Sort` alone and identifies boolean args by the `num_args = 0..=1` proxy rather than by their parser; both are addressed in a follow-up, now that `Arg::get_possible_values()` surfaces the boolish literal set. Note `crates/fgumi-cli-common` (on main-runall, not here) carries a second copy of `parse_bool` per the landing tracker's duplication ledger. It must not be reinstated as the live parser when the umbrella points at that crate. cargo ci-fmt/ci-lint clean; 6891 tests pass, 27 skipped. Help rendering verified on the built binary across sort, filter, dedup, group, clip, correct, and simplex.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
#690 added `test_bool_args_name_their_accepted_values` to `sort.rs`. It guarded `Sort` alone, and it identified boolean flags by the structural proxy `num_args = 0..=1` -- a shape an optional path or optional count shares, so the first non-boolean optional-value flag added to `Sort` would have failed it with a message telling the author to declare `value_name = "true|false"` on it. There was no way to do better against `parse_bool`: clap keeps `default_missing_vals` private with no getter, and `ArgAction` does not implement `PartialEq`. `BoolishValueParser` changes that. `Arg::get_possible_values()` surfaces the parser's own twelve literals (ten of them hidden), so `is_boolish` now decides exactly whether a flag is a boolean rather than inferring it from shape. That precision is what makes a repo-wide walk safe, so the guard moves to `src/main.rs` and recurses from `Args::command()` through every subcommand, including nested ones. It covers 44 flags across 19 commands instead of 3 on one, and it cannot drift from the CLI, because it walks the CLI rather than a parallel list. Four tests, each asserting a distinct half of the contract: - `..._name_their_accepted_values` -- every boolean flag declares `value_name = "true|false"`. - `..._hide_their_partial_possible_values` -- every boolean flag sets `hide_possible_values`, so help never advertises `true, false` alone. - `..._render_an_optional_boolean_placeholder` -- the rendered help of every command owning a boolean flag actually contains `[<true|false>]`. The declaration test cannot catch this on its own: the placeholder also depends on `num_args`, so changing `0..=1` to a required `1` would break bare `--verify` while still declaring the right `value_name`. - `..._top_level_help_names_every_accepted_spelling` -- ties the new top-level note to `BOOLISH_LITERALS`, so a change to the parser's set that leaves the documentation behind fails. Both vacuity guards are load-bearing and were verified by mutation: stripping `value_name` from `QueueMemoryOptions::memory_per_thread` fails the declaration and render tests naming `fgumi extract --memory_per_thread`, and stripping `hide_possible_values` fails the third. Since per-flag help names two of the twelve accepted spellings, the other ten are documented once -- in `fgumi --help` via `after_help`, and in the getting-started guide -- rather than repeated in 44 doc comments, where they would drift. cargo ci-fmt/ci-lint clean; 6894 tests pass, 27 skipped.
nh13
added a commit
that referenced
this pull request
Aug 7, 2026
#690 added `test_bool_args_name_their_accepted_values` to `sort.rs`. It guarded `Sort` alone, and it identified boolean flags by the structural proxy `num_args = 0..=1` -- a shape an optional path or optional count shares, so the first non-boolean optional-value flag added to `Sort` would have failed it with a message telling the author to declare `value_name = "true|false"` on it. There was no way to do better against `parse_bool`: clap keeps `default_missing_vals` private with no getter, and `ArgAction` does not implement `PartialEq`. `BoolishValueParser` changes that. `Arg::get_possible_values()` surfaces the parser's own twelve literals (ten of them hidden), so `is_boolish` now decides exactly whether a flag is a boolean rather than inferring it from shape. That precision is what makes a repo-wide walk safe, so the guard moves to `src/main.rs` and recurses from `Args::command()` through every subcommand, including nested ones. It covers 44 flags across 19 commands instead of 3 on one, and it cannot drift from the CLI, because it walks the CLI rather than a parallel list. Four tests, each asserting a distinct half of the contract: - `..._name_their_accepted_values` -- every boolean flag declares `value_name = "true|false"`. - `..._hide_their_partial_possible_values` -- every boolean flag sets `hide_possible_values`, so help never advertises `true, false` alone. - `..._render_an_optional_boolean_placeholder` -- the rendered help of every command owning a boolean flag actually contains `[<true|false>]`. The declaration test cannot catch this on its own: the placeholder also depends on `num_args`, so changing `0..=1` to a required `1` would break bare `--verify` while still declaring the right `value_name`. - `..._top_level_help_names_every_accepted_spelling` -- ties the new top-level note to `BOOLISH_LITERALS`, so a change to the parser's set that leaves the documentation behind fails. Both vacuity guards are load-bearing and were verified by mutation: stripping `value_name` from `QueueMemoryOptions::memory_per_thread` fails the declaration and render tests naming `fgumi extract --memory_per_thread`, and stripping `hide_possible_values` fails the third. Since per-flag help names two of the twelve accepted spellings, the other ten are documented once -- in `fgumi --help` via `after_help`, and in the getting-started guide -- rather than repeated in 44 doc comments, where they would drift. cargo ci-fmt/ci-lint clean; 6894 tests pass, 27 skipped.
Merged
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--memory-per-threadrendered as--memory-per-thread [<MEMORY_PER_THREAD>], because clap derives the value placeholder from the field name when novalue_nameis set. That reads as a request for a per-thread memory size and invites--memory-per-thread 10G. Short-hcompounds it, showing only the first doc line — which never says the flag is a toggle.Adds
value_name = "true|false"to the three optional-value booleans onSort:Help text only — parsing is unchanged, and the existing error already enumerates the accepted values. The test walks
Sort::command()and asserts every optional-value flag names its values.cargo ci-fmt/ci-lintclean;ci-testis 6890 passed / 3 failed, and those 3 (test_input_source_matrix::declared_{stdin,sam}_support_*) fail identically on a pristinemain— they diff R-generatedsimplex_qc.pdfbyte-for-byte and R stamps/CreationDateinto it. Unrelated, but probably worth its own issue.Follow-up: should we prefer
BoolishValueParser?We just used clap's
BoolishValueParserin dupblaster, and it may be the better default here than our ownparse_bool. Worth a separate PR, with two differences to weigh first:on|off|1|0on top of ourtrue|false|yes|no|y|n|t|f. Strictly backward compatible, but it diverges from sopt —OptionLookup.convertFlagValueaccepts exactly our current set, which is whatparse_bool's "matches sopt/fgbio" comment refers to.value was not a boolean, versus ourInvalid boolean value '10G'. Expected: true|false|yes|no|y|n|t|f. It also needshide_possible_values, or clap prints a misleading[possible values: true, false].So there's a third option: extend
parse_boolwith the boolish literals and keep the better error message. Either wayvalue_namebecomeson|off, so that decision and this one travel together.Summary by CodeRabbit
trueorfalsevalues.