Repository navigation
fix(cli): allow explicit true/false values on all boolean flags and remove dead code - #210
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #210 +/- ##
==========================================
- Coverage 88.06% 88.05% -0.02%
==========================================
Files 113 113
Lines 52820 52762 -58
==========================================
- Hits 46518 46460 -58
Misses 6302 6302 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nh13
marked this pull request as ready for review
March 31, 2026 05:53
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 18 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughThis PR standardizes boolean CLI flags across multiple command modules. Many flags were changed to accept an optional value via 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
![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
🧹 Nitpick comments (2)
src/commands/filter.rs (1)
150-170: Add parser coverage for these bool forms.The tests here validate runtime behavior, but they build
Filterdirectly, so they never exercise the CLI contract this change is introducing. A smallFilter::try_parse_frommatrix for bare /true/falsewould lock these flags down cheaply.As per coding guidelines, "Place unit tests alongside source code in src/ directory" and "Use rstest for parameterized tests".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/filter.rs` around lines 150 - 170, Add unit tests exercising the CLI parsing of the boolean flags by calling Filter::try_parse_from with a small parameterized matrix for the three forms (bare flag present, --flag=true, --flag=false) to verify reverse_per_base_tags, filter_by_template, and require_single_strand_agreement parse correctly; put tests next to src/commands/filter.rs using rstest to generate the combinations and assert the resulting Filter struct fields match expected booleans for each input vector, covering default, explicit true, and explicit false cases.src/commands/common.rs (1)
107-112: Add one wrapper-parser test for the shared optional bool args.These
Argsare only tested via direct struct construction right now, so the exact regression this PR is fixing can slip back in unnoticed. A tiny test-onlyParserwrapper covering bare /true/falsecases would pin the behavior once, especially for--queue-memory-per-thread false.As per coding guidelines, "Place unit tests alongside source code in src/ directory" and "Use rstest for parameterized tests".
Also applies to: 217-218, 454-455
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/common.rs` around lines 107 - 112, Add a unit test next to src/ that creates a tiny test-only clap Parser wrapper around the existing Args struct (the struct holding output_per_base_tags and trim) and use rstest to parameterize three cases for each optional-boolean flag: absent (bare), "--flag true", and "--flag false" (e.g., test for output_per_base_tags, trim and similarly for --queue-memory-per-thread), asserting the parsed field values match expected booleans; place tests in src/ and derive clap::Parser on the wrapper so the clap parsing behavior is exercised rather than constructing Args directly.
🤖 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/clip.rs`:
- Around line 119-120: The regenerate_tags boolean is parsed but ignored: code
paths that currently log and perform tag regeneration must be guarded by the new
flag; wrap the unconditional logging and regeneration calls with a check of
self.regenerate_tags (e.g. if self.regenerate_tags { ... }) so both the log
message and the actual tag-regeneration code only run when the flag is true, and
update the log text to reflect the actual value of self.regenerate_tags instead
of always stating regeneration occurred. Ensure every place that previously
unconditionally invoked tag regeneration uses this guard and that no hidden flag
remains disableable but ignored.
---
Nitpick comments:
In `@src/commands/common.rs`:
- Around line 107-112: Add a unit test next to src/ that creates a tiny
test-only clap Parser wrapper around the existing Args struct (the struct
holding output_per_base_tags and trim) and use rstest to parameterize three
cases for each optional-boolean flag: absent (bare), "--flag true", and "--flag
false" (e.g., test for output_per_base_tags, trim and similarly for
--queue-memory-per-thread), asserting the parsed field values match expected
booleans; place tests in src/ and derive clap::Parser on the wrapper so the clap
parsing behavior is exercised rather than constructing Args directly.
In `@src/commands/filter.rs`:
- Around line 150-170: Add unit tests exercising the CLI parsing of the boolean
flags by calling Filter::try_parse_from with a small parameterized matrix for
the three forms (bare flag present, --flag=true, --flag=false) to verify
reverse_per_base_tags, filter_by_template, and require_single_strand_agreement
parse correctly; put tests next to src/commands/filter.rs using rstest to
generate the combinations and assert the resulting Filter struct fields match
expected booleans for each input vector, covering default, explicit true, and
explicit false cases.
🪄 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: b52b5017-5d9e-4370-a4f6-50c1d1521ad6
📒 Files selected for processing (11)
src/commands/clip.rssrc/commands/common.rssrc/commands/compare/bams.rssrc/commands/dedup.rssrc/commands/downsample.rssrc/commands/duplex_metrics.rssrc/commands/fastq.rssrc/commands/filter.rssrc/commands/review.rssrc/commands/sort.rssrc/commands/zipper.rs
nh13
changed the title
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
src/commands/zipper.rs (1)
168-176: Consider parser-level tests for these two flags.You already validate runtime behavior; adding CLI parse tests for bare and equals forms would lock in this contract (
--flag,--flag=true,--flag=false).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/zipper.rs` around lines 168 - 176, Add parser-level unit tests that verify CLI parsing for the boolean flags represented by the fields exclude_missing_reads and skip_pa_tags: cover absent (default false), bare flag form (--exclude-missing-reads, --skip-pa-tags), and equals forms (--exclude-missing-reads=true/false, --skip-pa-tags=true/false). Locate the argument parser instantiation used to populate those fields (the clap-derived struct or function that parses args for zipper) and add tests that call that parser with the different argv variants and assert the resulting struct fields are true/false as expected. Ensure tests exercise both the short/bare and explicit equals syntaxes to lock the CLI contract.src/commands/common.rs (1)
1122-1164: Add--flag=<bool>cases to lock the exact contract.Current tests cover
--flag false, but not the explicit equals form (--flag=false) called out in the PR objective.Suggested test additions
#[case(&["test", "--output-per-base-tags", "false"], false)] +#[case(&["test", "--output-per-base-tags=false"], false)] fn test_output_per_base_tags_parsing(#[case] args: &[&str], #[case] expected: bool) { #[case(&["test", "--trim", "false"], false)] +#[case(&["test", "--trim=false"], false)] fn test_trim_parsing(#[case] args: &[&str], #[case] expected: bool) { #[case(&["test", "--consensus-call-overlapping-bases", "false"], false)] +#[case(&["test", "--consensus-call-overlapping-bases=false"], false)] fn test_overlapping_bases_parsing(#[case] args: &[&str], #[case] expected: bool) { #[case(&["test", "--queue-memory-per-thread", "false"], false)] +#[case(&["test", "--queue-memory-per-thread=false"], false)] fn test_queue_memory_per_thread_parsing(#[case] args: &[&str], #[case] expected: bool) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/common.rs` around lines 1122 - 1164, The boolean-flag parsing tests (test_output_per_base_tags_parsing, test_trim_parsing, test_overlapping_bases_parsing, test_queue_memory_per_thread_parsing) currently check space-separated and positional "false"/"true" forms but miss the explicit equals form (--flag=false / --flag=true); update each TestBoolFlags invocation to add cases with the equals syntax (e.g. "--output-per-base-tags=false" and "--output-per-base-tags=true" for consensus.output_per_base_tags; "--trim=false"/"=true" for consensus.trim; "--consensus-call-overlapping-bases=false"/"=true" for overlapping.consensus_call_overlapping_bases; and "--queue-memory-per-thread=false"/"=true" for queue_memory.queue_memory_per_thread) so the tests lock the exact CLI contract.src/commands/filter.rs (1)
3952-3981: Add--flag=<bool>cases and removecontains(...)dispatch in the test.Current cases don’t directly cover the exact
--flag=false/--flag=trueform from the PR goal, and thecontains-based branch on Line 3969 is fragile.Proposed test hardening
#[rstest] -#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--reverse-per-base-tags"], true)] -#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--reverse-per-base-tags", "true"], true)] -#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--reverse-per-base-tags", "false"], false)] -#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1"], false)] +#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1"], false, true, false)] +#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--reverse-per-base-tags"], true, true, false)] +#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--reverse-per-base-tags=true"], true, true, false)] +#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--reverse-per-base-tags=false"], false, true, false)] #[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--filter-by-template"], true)] -#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--filter-by-template", "true"], true)] -#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--filter-by-template", "false"], false)] +#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--filter-by-template=true"], false, true, false)] +#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--filter-by-template=false"], false, false, false)] #[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--require-single-strand-agreement"], true)] -#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--require-single-strand-agreement", "false"], false)] -fn test_bool_flag_parsing(#[case] args: &[&str], #[case] expected: bool) { +#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--require-single-strand-agreement=true"], false, true, true)] +#[case(&["filter", "-i", "in.bam", "-o", "out.bam", "-M", "1", "--require-single-strand-agreement=false"], false, true, false)] +fn test_bool_flag_parsing( + #[case] args: &[&str], + #[case] expected_reverse: bool, + #[case] expected_template: bool, + #[case] expected_ssa: bool, +) { let cmd = Filter::try_parse_from(args).unwrap(); - // Determine which flag was under test by checking which non-default args are present - if args.iter().any(|a| a.contains("reverse-per-base-tags")) { - assert_eq!(cmd.reverse_per_base_tags, expected); - } else if args.iter().any(|a| a.contains("filter-by-template")) { - assert_eq!(cmd.filter_by_template, expected); - } else if args.iter().any(|a| a.contains("require-single-strand-agreement")) { - assert_eq!(cmd.require_single_strand_agreement, expected); - } else { - // Default case (no bool flags specified): check defaults - assert!(!cmd.reverse_per_base_tags); - assert!(cmd.filter_by_template); - assert!(!cmd.require_single_strand_agreement); - } + assert_eq!(cmd.reverse_per_base_tags, expected_reverse); + assert_eq!(cmd.filter_by_template, expected_template); + assert_eq!(cmd.require_single_strand_agreement, expected_ssa); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/filter.rs` around lines 3952 - 3981, The test test_bool_flag_parsing is fragile because it uses args.iter().any(|a| a.contains(...)) to decide which flag to assert and it lacks explicit --flag=true/false forms; update the cases to include exact "--reverse-per-base-tags=true"/"--reverse-per-base-tags=false", "--filter-by-template=true"/"--filter-by-template=false", and "--require-single-strand-agreement=true"/"...=false" variants, and replace the contains-based dispatch with explicit per-case identification (e.g., add a third #[case_flag] parameter or encode the flag name in the case) so the test directly asserts the corresponding Filter field (reverse_per_base_tags, filter_by_template, require_single_strand_agreement) after parsing via Filter::try_parse_from(args).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/commands/common.rs`:
- Around line 1122-1164: The boolean-flag parsing tests
(test_output_per_base_tags_parsing, test_trim_parsing,
test_overlapping_bases_parsing, test_queue_memory_per_thread_parsing) currently
check space-separated and positional "false"/"true" forms but miss the explicit
equals form (--flag=false / --flag=true); update each TestBoolFlags invocation
to add cases with the equals syntax (e.g. "--output-per-base-tags=false" and
"--output-per-base-tags=true" for consensus.output_per_base_tags;
"--trim=false"/"=true" for consensus.trim;
"--consensus-call-overlapping-bases=false"/"=true" for
overlapping.consensus_call_overlapping_bases; and
"--queue-memory-per-thread=false"/"=true" for
queue_memory.queue_memory_per_thread) so the tests lock the exact CLI contract.
In `@src/commands/filter.rs`:
- Around line 3952-3981: The test test_bool_flag_parsing is fragile because it
uses args.iter().any(|a| a.contains(...)) to decide which flag to assert and it
lacks explicit --flag=true/false forms; update the cases to include exact
"--reverse-per-base-tags=true"/"--reverse-per-base-tags=false",
"--filter-by-template=true"/"--filter-by-template=false", and
"--require-single-strand-agreement=true"/"...=false" variants, and replace the
contains-based dispatch with explicit per-case identification (e.g., add a third
#[case_flag] parameter or encode the flag name in the case) so the test directly
asserts the corresponding Filter field (reverse_per_base_tags,
filter_by_template, require_single_strand_agreement) after parsing via
Filter::try_parse_from(args).
In `@src/commands/zipper.rs`:
- Around line 168-176: Add parser-level unit tests that verify CLI parsing for
the boolean flags represented by the fields exclude_missing_reads and
skip_pa_tags: cover absent (default false), bare flag form
(--exclude-missing-reads, --skip-pa-tags), and equals forms
(--exclude-missing-reads=true/false, --skip-pa-tags=true/false). Locate the
argument parser instantiation used to populate those fields (the clap-derived
struct or function that parses args for zipper) and add tests that call that
parser with the different argv variants and assert the resulting struct fields
are true/false as expected. Ensure tests exercise both the short/bare and
explicit equals syntaxes to lock the CLI contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f1207d68-734c-487d-8a61-5093712617a9
📒 Files selected for processing (11)
src/commands/clip.rssrc/commands/common.rssrc/commands/compare/bams.rssrc/commands/dedup.rssrc/commands/downsample.rssrc/commands/duplex_metrics.rssrc/commands/fastq.rssrc/commands/filter.rssrc/commands/review.rssrc/commands/sort.rssrc/commands/zipper.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/commands/compare/bams.rs
- src/commands/downsample.rs
- src/commands/duplex_metrics.rs
- src/commands/sort.rs
- src/commands/fastq.rs
- src/commands/dedup.rs
- src/commands/clip.rs
Boolean flags using `default_value` with clap's default `SetTrue` action rejected explicit values like `--flag true` or `--flag false`. This was especially broken for 5 flags defaulting to true (--output-per-base-tags, --consensus-call-overlapping-bases, --queue-memory-per-thread, --filter-by-template, --regenerate-tags) which were stuck permanently on with no way to disable them. Adds `action = ArgAction::Set, num_args = 0..=1, default_missing_value = "true"` to all 22 boolean flags, matching sopt/fgbio behavior where both `--flag` (implies true) and `--flag=false` (explicit) are accepted. Backwards compatible: existing `--flag` usage continues to work.
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
1 participant
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.
Summary
--flag=trueand--flag=falsevalues, matching fgbio/sopt behavior. Previously, flags usingArgAction::SetTruewithdefault_value = "true"were stuck permanently on — passing--flag falseor--flag=falsewould error or be ignored.--regenerate-tagsflag from theclipcommand entirely. This was dead code — it was hidden, always true, and had no way to disable it. fgbio'sClipBamnever had this flag either; NM/UQ/MD tag regeneration is unconditional.=true,=false, space-separated, and default forms.Changes
Boolean flag fix (all commands):
actionfromArgAction::SetTrue/SetFalsetoArgAction::Setnum_args = 0..=1anddefault_missing_valueso bare--flagstill worksclip,codec,common(consensus/overlapping/queue-memory options),compare/bams,dedup,downsample,duplex-metrics,fastq,filter,review,simplex,sort,zipperDead code removal (
clip):--regenerate-tagsfield, log line, doc string reference, and all test referencesNew tests:
common.rs: 24 rstest cases covering--output-per-base-tags,--trim,--consensus-call-overlapping-bases,--queue-memory-per-thread(bare,=true,=false, space-separated, default)filter.rs: 18 rstest cases in 3 separate per-flag functions covering--reverse-per-base-tags,--filter-by-template,--require-single-strand-agreementzipper.rs: 12 rstest cases covering--exclude-missing-reads,--skip-pa-tagsTest plan
cargo ci-test— all 2052 tests passcargo ci-fmt— cleancargo ci-lint— clean--flag(bare) still works as before--flag=falsenow correctly disables flags that default to true--flag=trueequals form works identically to space-separated form