feat(cli): accept on/off/1/0 for boolean flags via BoolishValueParser

[nh13]

Follow-up to the question raised in #690: should we prefer BoolishValueParser?

What changes

parse_bool is replaced by clap's BoolishValueParser at all 44 boolean #[arg] sites, and the function is deleted. Each of those 44 sites also gains value_name = "true|false", extending repo-wide the convention #690 established for Sort's three flags.

	accepted (case-insensitive)
before	true false yes no y n t f
after	those, plus on off 1 0

A strict superset, verified against clap_builder-4.6.2: TRUE_LITERALS and FALSE_LITERALS contain all eight previous spellings. Nothing that parsed before parses differently.

Unrecognized input is still rejected. Worth stating explicitly because clap's own doc comment on str_to_bool claims "Any other value will be considered as true" — that comment is wrong. The function returns None, and BoolishValueParser::parse_ref maps that to a validation error. Confirmed on the binary: --memory-per-thread 10G errors.

The tradeoff, stated plainly

parse_bool documented itself as "Matches sopt/fgbio behavior", so this deliberately diverges from sopt's accepted set — that is the one real consequence, and it is the point of the change rather than a side effect. The flags now take the spellings users coming from other CLIs reach for first.

The other cost named in #690 was the error message. Ours was Invalid boolean value '10G'. Expected: true|false|yes|no|y|n|t|f; clap's is invalid value '10G' for '--memory-per-thread [<true|false>]': value was not a boolean. Less enumerative, but it names the offending flag, which ours never did.

hide_possible_values and the placeholder

These two are one decision, which is why the value_name sweep rides here rather than in a follow-up.

BoolishValueParser::possible_values() yields all twelve literals but marks ten hidden, so help would print [possible values: true, false] — advertising two of the twelve accepted spellings, which is worse than advertising none. Each site therefore sets hide_possible_values = true.

That suppression makes the placeholder the only in-help signal about what the flag takes, and without value_name clap derives it 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 that for the three flags on Sort; the remaining 41 were left rendering the derived name, so the same flag rendered two different 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 still: it set value_name = "ALLOW_UNMAPPED" explicitly.

Verified on the built binary across sort, filter, dedup, group, clip, correct, and simplex: every one now renders --memory-per-thread [<true|false>], no [possible values: ...] line appears anywhere, and no upper-snake placeholder survives on a boolean flag.

Tests

The two rstest tables that called parse_bool directly now drive the same cases through TestBoolFlags::try_parse_from, asserting the CLI contract rather than a private function, so they 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 rather than implied — a third table, test_extended_bool_values_in_cli_invalid, asserted those four were rejected, and it failed on the first run of this change exactly as it should have.

test_bool_args_name_their_accepted_values (added by #690) still guards Sort only, and it identifies boolean args by the structural proxy num_args = 0..=1 rather than by their parser. Both are addressed in a follow-up PR stacked on this one: Arg::get_possible_values() now surfaces BoolishValueParser's literal set, so the test can identify boolean args exactly and walk every command instead of Sort.

cargo ci-fmt / ci-lint clean.

Note for the runall landing series

feat-runall and main-runall are deliberately not changed here. main-runall inherits this for src/lib/commands/ on its next rebase, but two copies of parse_bool survive on that side and must not become the live parser — crates/fgumi-cli-common/src/lib.rs:526 (landed by #672) and crates/fgumi-sort-cli/src/sort.rs (lands at P4). Both are recorded in the landing tracker's duplication ledger so the port converts them rather than reintroducing the function.

Risk: command output changes: none; unsafe changes: none, and CLAUDE.md allowlist updates: none; memory bounds, queue capacity, and thread/backpressure policy changes: none.

Replaced the custom parse_bool parser at 44 CLI boolean argument sites with clap’s BoolishValueParser.

Boolean arguments now accept true, false, yes, no, y, n, t, f, on, off, 1, and 0. Invalid values remain rejected.

Standardized boolean help output with value_name = "true|false" and hidden possible values.

Updated CLI parsing tests for the expanded value set and invalid input. Formatting, lint, tests, and help rendering pass.

[coderabbitai]

Note

Reviews paused

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d2833aff-4f5f-4fe0-9369-7646dae34726

📥 Commits

Reviewing files that changed from the base of the PR and between 25d8d6d and 2dbfbb7.

⛔ Files ignored due to path filters (1)

• CHANGELOG.md is excluded by !**/CHANGELOG.md

📒 Files selected for processing (18)

• src/lib/commands/clip.rs
• src/lib/commands/common.rs
• src/lib/commands/compare/bams.rs
• src/lib/commands/compare/metrics.rs
• src/lib/commands/correct.rs
• src/lib/commands/dedup.rs
• src/lib/commands/downsample.rs
• src/lib/commands/duplex_metrics.rs
• src/lib/commands/fastq.rs
• src/lib/commands/filter.rs
• src/lib/commands/group.rs
• src/lib/commands/review.rs
• src/lib/commands/simulate/common.rs
• src/lib/commands/simulate/consensus_reads.rs
• src/lib/commands/simulate/fastq_reads.rs
• src/lib/commands/simulate/grouped_reads.rs
• src/lib/commands/sort.rs
• src/lib/commands/zipper.rs

---

Walkthrough

The PR replaces the removed parse_bool helper with Clap’s BoolishValueParser across CLI commands. It preserves optional values and defaults, updates boolean help metadata, and expands CLI-level parsing tests.

Changes

CLI boolean parser migration

Layer / File(s)	Summary
Common parser and CLI validation src/lib/commands/common.rs	Removes parse_bool, updates shared boolean options, and tests accepted and rejected CLI values.
Read-processing command options src/lib/commands/clip.rs, src/lib/commands/correct.rs, src/lib/commands/dedup.rs, src/lib/commands/downsample.rs, src/lib/commands/duplex_metrics.rs, src/lib/commands/fastq.rs	Replaces custom boolean parsing with BoolishValueParser while retaining existing option behavior.
Comparison, filtering, and grouping options src/lib/commands/compare/*, src/lib/commands/filter.rs, src/lib/commands/group.rs, src/lib/commands/review.rs	Migrates command-specific boolean options and removes obsolete imports.
Sorting, zipper, and simulation options src/lib/commands/sort.rs, src/lib/commands/zipper.rs, src/lib/commands/simulate/*	Migrates remaining boolean options and updates related helper documentation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

• fulcrumgenomics/fgumi#617: Modifies the Zipper command’s skip_tc_tags option.
• fulcrumgenomics/fgumi#672: Introduces a shared parse_bool implementation.
• fulcrumgenomics/fgumi#690: Modifies the same Sort boolean options.

Suggested labels: fgumi group, fgumi sort

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Title check	⚠️ Warning	The title describes the main CLI change and uses conventional-commit syntax, but the cli scope does not name an affected command or crate.	Remove the generic scope or replace it with an affected command or crate, such as feat: accept on/off/1/0 for boolean flags via BoolishValueParser.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

_{Comment @coderabbitai help to get the list of available commands.}

[nh13]

@coderabbitai pause

[coderabbitai]

✅ Action performed

Reviews paused.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.06%. Comparing base (25d8d6d) to head (2dbfbb7).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #703   +/-   ##
=======================================
  Coverage   94.06%   94.06%           
=======================================
  Files         178      178           
  Lines      108601   108595    -6     
=======================================
- Hits       102153   102151    -2     
+ Misses       6448     6444    -4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.