Skip to content

Accept yes/no/y/n/t/f as boolean flag values - #235

Merged
nh13 merged 1 commit into
mainfrom
nh13/feat-parse-bool
Apr 6, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh13/feat-parse-bool

Conversation

@nh13

@nh13 nh13 commented Apr 6, 2026

Copy link
Copy Markdown
Member

Summary

  • Adds a custom parse_bool value parser in common.rs that accepts true/false/yes/no/y/n/t/f (case-insensitive), matching sopt/fgbio boolean flag behavior
  • Wires value_parser = parse_bool into all boolean CLI flags across 17 files
  • Adds the full num_args = 0..=1 / default_missing_value = "true" / ArgAction::Set pattern to 17 flags that were still using clap's default SetTrue action (simulate, group, correct, sort, compare, and hidden scheduler flags)
  • Every boolean flag in the CLI now consistently supports: --flag (bare), --flag true/false, --flag yes/no, --flag=t/f, etc.

Test plan

  • Unit tests for parse_bool: 23 valid cases (all accepted values in various casings), 13 invalid cases (typos, nonsense, whitespace)
  • Integration tests for extended values through clap: 10 valid CLI cases, 5 invalid CLI cases
  • All existing boolean flag parsing tests continue to pass
  • Full test suite: 2283 tests pass, 0 failures
  • cargo ci-fmt and cargo ci-lint clean

@nh13
nh13 temporarily deployed to github-actions April 6, 2026 06:03 — with GitHub Actions Inactive
@codecov

codecov Bot commented Apr 6, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.98%. Comparing base (7c1cc80) to head (1d36434).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #235   +/-   ##
=======================================
  Coverage   88.97%   88.98%           
=======================================
  Files         113      113           
  Lines       55038    55065   +27     
=======================================
+ Hits        48970    48998   +28     
+ Misses       6068     6067    -1     

☔ View full report in Codecov by Sentry.
📢 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.

@coderabbitai

coderabbitai Bot commented Apr 6, 2026 •

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4a13c38d-d6b7-4a41-92ff-13da42742a62

📥 Commits

Reviewing files that changed from the base of the PR and between 9dc160d and 1d36434.

📒 Files selected for processing (17)
  • src/commands/clip.rs
  • src/commands/common.rs
  • src/commands/compare/bams.rs
  • src/commands/compare/metrics.rs
  • src/commands/correct.rs
  • src/commands/dedup.rs
  • src/commands/downsample.rs
  • src/commands/duplex_metrics.rs
  • src/commands/fastq.rs
  • src/commands/filter.rs
  • src/commands/group.rs
  • src/commands/review.rs
  • src/commands/simulate/consensus_reads.rs
  • src/commands/simulate/fastq_reads.rs
  • src/commands/simulate/grouped_reads.rs
  • src/commands/sort.rs
  • src/commands/zipper.rs
✅ Files skipped from review due to trivial changes (1)
  • src/commands/filter.rs
🚧 Files skipped from review as they are similar to previous changes (9)
  • src/commands/review.rs
  • src/commands/clip.rs
  • src/commands/compare/bams.rs
  • src/commands/simulate/grouped_reads.rs
  • src/commands/zipper.rs
  • src/commands/compare/metrics.rs
  • src/commands/correct.rs
  • src/commands/common.rs
  • src/commands/group.rs

📝 Walkthrough

Walkthrough

Added pub(crate) fn parse_bool(s: &str) -> Result<bool, String> in src/commands/common.rs for case-insensitive boolean parsing (true/false, t/f, yes/no, y/n) and extended unit tests. Updated many CLI boolean flags across command modules to use value_parser = parse_bool and, where applicable, to allow optional values (num_args = 0..=1, default_missing_value = "true", ArgAction::Set). In sort removed some clap conflicts_with rules and introduced runtime checks plus tests that error when conflicting flags are used.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly describes the main change: adding support for additional boolean flag value formats (yes/no/y/n/t/f) alongside true/false.
Description check ✅ Passed Description is directly related to the changeset, detailing the parse_bool implementation, affected files, test coverage, and expected behavior.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nh13/feat-parse-bool

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/commands/common.rs (1)

628-634: Consider making parse_bool crate-private.

This reads like an internal clap helper. If external crates are not meant to call it, pub(crate) keeps the public API surface smaller.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/common.rs` around lines 628 - 634, The parse_bool helper is
currently exported publicly but intended as an internal clap helper; change its
visibility from pub to pub(crate) on the function declaration for parse_bool to
keep it crate-private, and update any internal references if necessary (search
for parse_bool usages) to ensure they remain in-crate; no other behavioral
changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/sort.rs`:
- Around line 172-173: Remove the attribute-level conflicts on the verify flag
(the conflicts_with settings on the verify field which, combined with
ArgAction::Set, treat --verify=false as "present") and instead add runtime
validation in the command's execute() method: check if self.verify is true and
if so bail with clear errors when self.output.is_some() or self.write_index is
true (e.g., "--verify cannot be used with --output" and "--write-index cannot be
used with --verify"). Ensure the checks reference the verify field, output
option and write_index flag and run before proceeding with the rest of
execute().

---

Nitpick comments:
In `@src/commands/common.rs`:
- Around line 628-634: The parse_bool helper is currently exported publicly but
intended as an internal clap helper; change its visibility from pub to
pub(crate) on the function declaration for parse_bool to keep it crate-private,
and update any internal references if necessary (search for parse_bool usages)
to ensure they remain in-crate; no other behavioral changes are needed.
🪄 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 3a25df5d-ff43-4b82-90eb-37edf063b47f

📥 Commits

Reviewing files that changed from the base of the PR and between 7c1cc80 and 9dc160d.

📒 Files selected for processing (17)
  • src/commands/clip.rs
  • src/commands/common.rs
  • src/commands/compare/bams.rs
  • src/commands/compare/metrics.rs
  • src/commands/correct.rs
  • src/commands/dedup.rs
  • src/commands/downsample.rs
  • src/commands/duplex_metrics.rs
  • src/commands/fastq.rs
  • src/commands/filter.rs
  • src/commands/group.rs
  • src/commands/review.rs
  • src/commands/simulate/consensus_reads.rs
  • src/commands/simulate/fastq_reads.rs
  • src/commands/simulate/grouped_reads.rs
  • src/commands/sort.rs
  • src/commands/zipper.rs

Comment thread src/commands/sort.rs Outdated
Add a custom `parse_bool` value parser that accepts true/false, yes/no,
y/n, and t/f (case-insensitive), matching sopt/fgbio behavior. Wire it
into all boolean CLI flags via `value_parser = parse_bool`.

Also adds the full `num_args = 0..=1` / `default_missing_value` /
`ArgAction::Set` pattern to 17 boolean flags that were still using
clap's default `SetTrue` action (simulate commands, group, correct,
sort, compare, and hidden scheduler flags).
@nh13 nh13 added the enhancement New feature or request label Apr 6, 2026
@nh13
nh13 force-pushed the nh13/feat-parse-bool branch from 9dc160d to 1d36434 Compare April 6, 2026 06:48
@nh13
nh13 temporarily deployed to github-actions April 6, 2026 06:48 — with GitHub Actions Inactive
@nh13
nh13 merged commit 2f14de1 into main Apr 6, 2026
8 checks passed
@nh13
nh13 deleted the nh13/feat-parse-bool branch April 6, 2026 06:58
@nh13 nh13 mentioned this pull request Apr 6, 2026

This branch was previously deployed

1 inactive deployment
github-actions — 1d364348 Deployed Apr 6, 2026 by nh13 via coverage #927
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant