test(cli): guard boolean-flag help across every command - #704
Conversation
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe CLI now documents accepted boolean flag spellings and optional values. Repository-wide tests validate boolean metadata, help placeholders, recursive command coverage, and bare flags. The getting-started guide documents the same behavior. ChangesBoolean flag help and validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #704 +/- ##
=======================================
Coverage 94.03% 94.03%
=======================================
Files 178 178
Lines 108829 108899 +70
=======================================
+ Hits 102340 102408 +68
- Misses 6489 6491 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai pause |
✅ Action performedReviews paused. |
c61ed4f to
fbd9db1
Compare
af7d9a2 to
38c12d8
Compare
fbd9db1 to
2dbfbb7
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/main.rs`:
- Around line 72-76: Wrap the Rust documentation identifier BoolishValueParser
in backticks at all four affected sites: src/main.rs lines 72-76, 237-241,
265-270, and 340-345. No other documentation content needs to change.
- Around line 376-405: Expand
test_bool_args_render_an_optional_boolean_placeholder to iterate over each
boolean flag rather than only deduplicated command paths. For every flag,
resolve its command, assert help contains the optional [<BOOL_VALUE_NAME>]
placeholder, and parse the flag’s bare long option to verify it yields true.
🪄 Autofix
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
Run ID: 4a87531c-f95c-4863-9ca8-d8cec9283c7e
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!**/CHANGELOG.md
📒 Files selected for processing (20)
docs/src/guide/getting-started.mdsrc/lib/commands/clip.rssrc/lib/commands/common.rssrc/lib/commands/compare/bams.rssrc/lib/commands/compare/metrics.rssrc/lib/commands/correct.rssrc/lib/commands/dedup.rssrc/lib/commands/downsample.rssrc/lib/commands/duplex_metrics.rssrc/lib/commands/fastq.rssrc/lib/commands/filter.rssrc/lib/commands/group.rssrc/lib/commands/review.rssrc/lib/commands/simulate/common.rssrc/lib/commands/simulate/consensus_reads.rssrc/lib/commands/simulate/fastq_reads.rssrc/lib/commands/simulate/grouped_reads.rssrc/lib/commands/sort.rssrc/lib/commands/zipper.rssrc/main.rs
#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.
38c12d8 to
249532d
Compare
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Stacked on #703 — base is
nh/cli-boolish-value-parser, notmain. Review #703 first; the diff shown here is only the test and documentation work.Closes out the review comments on #690 that #703 did not, now that switching to
BoolishValueParsermakes the precise version possible.The problem with the guard as it stands
#690 added
test_bool_args_name_their_accepted_valuestosort.rs. It has two limits:Sortalone, while the convention it enforces is repo-wide.num_args = 0..=1. An optional path or an optional count has that same shape, so the first non-boolean optional-value flag added toSortwould fail the test with a message instructing the author to declarevalue_name = "true|false"on it — wrong advice, produced confidently.Against
parse_boolthere was no better option: clap keepsdefault_missing_valsprivate with no getter (clap_builder-4.6.2/src/builder/arg.rs:85), andArgActiondoes not implementPartialEq(builder/action.rs:31), so neither discriminator was reachable.What #703 unlocked
BoolishValueParserimplementspossible_values(), andArg::get_possible_values()(builder/arg.rs:4320) surfaces it — all twelve literals, including the ten clap marks hidden. Sois_boolishcan now decide exactly whether a flag is a boolean instead of inferring it from shape.That precision is what makes a repo-wide walk safe. The guard moves to
src/main.rsand recurses fromArgs::command()through every subcommand, nested ones included: 44 flags across 19 commands instead of 3 on one. It walks the CLI itself rather than a parallel list, so it cannot drift from it.Four tests, four halves of the contract
..._name_their_accepted_valuesvalue_name = "true|false"..._hide_their_partial_possible_valueshide_possible_values, so help never advertisestrue, falsealone..._render_an_optional_boolean_placeholder[<true|false>]..._top_level_help_names_every_accepted_spellingBOOLISH_LITERALSThe render test is not redundant with the declaration test. The placeholder also depends on
num_args: changing0..=1to a required1would break bare--verifywhile still declaring the correctvalue_name, and only the render test sees that.Vacuity guards, verified by mutation
Both guards in
bool_flags()— a>= 40floor and a check that the walk reached six named commands — exist because a test that walks an empty set passes. They were verified rather than assumed:value_namefromQueueMemoryOptions::memory_per_threadfails the declaration and render tests, namingfgumi extract --memory_per_thread;hide_possible_valuesfrom the same flag fails the third.Documenting the other ten spellings
The remaining #690 comment was that
true|falsenames two of the twelve values the parser accepts, and that after #703 nothing in the CLI names the rest — clap's error isvalue was not a boolean, which does not enumerate.I did not put this in each flag's doc comment, which is what that comment literally asked for. Forty-four copies of the same sentence is forty-four places to drift, and the sentence is a property of the shared parser rather than of any one flag. It is stated once in
fgumi --helpviaafter_help, once in the getting-started guide, and pinned toBOOLISH_LITERALSby the fourth test. Say the word if you want it per-flag instead.Tests
cargo ci-fmt/ci-lintclean.Risk: CLI help output changes only, pinned by repository-wide boolean-flag tests; unsafe changes: none; memory bounds, queue capacity, and thread/backpressure policy changes: none.
sort.rstosrc/main.rs.cargo ci-fmtandci-lintpass.