Repository navigation
refactor: replace all unwrap() calls in crates/ with expect() - #225
Merged
Merged
Conversation
Audit and replace all 494 unwrap() calls across 36 files in crates/. Production code (~61 calls) converted to expect() with invariant messages. Test code (~433 calls) converted to expect() with context-specific failure messages. Only 9 unwrap() calls remain, all in doc comments where they are standard Rust convention.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #225 +/- ##
=======================================
Coverage 88.18% 88.18%
=======================================
Files 113 113
Lines 53388 53388
=======================================
+ Hits 47080 47081 +1
+ Misses 6308 6307 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR replaces numerous 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/fgumi-metrics/src/consensus.rs (1)
492-534: LGTM on the unwrap→expect changes.Note: The
assert!(x.is_some())checks before.expect()are redundant—both panic onNone. Consider removing them in a future cleanup.,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-metrics/src/consensus.rs` around lines 492 - 534, The test redundantly calls assert!(...is_some()) before using expect(...) on the same Option values (kv_metrics.iter().find(...)); remove the duplicate assert!(...). Keep the expect(...) calls which already provide a clear panic message and assertions on .value/.description for keys like "raw_reads_considered", "raw_reads_rejected", "raw_reads_used", "frac_raw_reads_used", "consensus_reads_emitted", "raw_reads_rejected_for_insufficient_support", and "raw_reads_rejected_for_minority_alignment" so the test remains concise and still fails with the same diagnostics if a metric is missing.crates/fgumi-consensus/src/duplex_caller.rs (1)
375-375: Tighten a fewexpectmessages to reflect the actual invariant.A few messages are still generic (“expected item at index”, “expected another item from iterator”). At Line 2312/Line 2316 these are keyed lookups, and at Line 5080 it’s really “exactly one record expected”.
Suggested message-only cleanup
- let last = *min_reads.last().expect("expected a last item"); + let last = *min_reads + .last() + .expect("min_reads must be non-empty (validated above)"); - let (a_reads, b_reads) = groups.get("UMI1").expect("expected item at index"); + let (a_reads, b_reads) = groups + .get("UMI1") + .expect("expected group for key UMI1"); - let (a_reads, b_reads) = groups.get("UMI2").expect("expected item at index"); + let (a_reads, b_reads) = groups + .get("UMI2") + .expect("expected group for key UMI2"); - records.into_iter().next().expect("expected another item from iterator") + records + .into_iter() + .next() + .expect("expected exactly one parsed record")Also applies to: 2312-2317, 5077-5080
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-consensus/src/duplex_caller.rs` at line 375, Replace the generic expect messages with precise invariants: for the shown line change the panic text on min_reads.last().expect(...) to something like "min_reads must contain at least one element" (referencing the min_reads variable in duplex_caller.rs); for the keyed lookups mentioned (the HashMap/lookup sites around the other occurrences) update their expect messages to include the missing key or context (e.g., "expected entry for key {key} in <variable_name>"); and for the case noted as "exactly one record expected" replace that generic expect with "exactly one record expected in <collection_name>, found {len}" so the panic communicates the exact invariant and context. Ensure each expect uses the actual variable/key names from duplex_caller.rs to make the messages actionable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/fgumi-consensus/src/duplex_caller.rs`:
- Line 375: Replace the generic expect messages with precise invariants: for the
shown line change the panic text on min_reads.last().expect(...) to something
like "min_reads must contain at least one element" (referencing the min_reads
variable in duplex_caller.rs); for the keyed lookups mentioned (the
HashMap/lookup sites around the other occurrences) update their expect messages
to include the missing key or context (e.g., "expected entry for key {key} in
<variable_name>"); and for the case noted as "exactly one record expected"
replace that generic expect with "exactly one record expected in
<collection_name>, found {len}" so the panic communicates the exact invariant
and context. Ensure each expect uses the actual variable/key names from
duplex_caller.rs to make the messages actionable.
In `@crates/fgumi-metrics/src/consensus.rs`:
- Around line 492-534: The test redundantly calls assert!(...is_some()) before
using expect(...) on the same Option values (kv_metrics.iter().find(...));
remove the duplicate assert!(...). Keep the expect(...) calls which already
provide a clear panic message and assertions on .value/.description for keys
like "raw_reads_considered", "raw_reads_rejected", "raw_reads_used",
"frac_raw_reads_used", "consensus_reads_emitted",
"raw_reads_rejected_for_insufficient_support", and
"raw_reads_rejected_for_minority_alignment" so the test remains concise and
still fails with the same diagnostics if a metric is missing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b1480bba-6a96-424a-9d93-fe17453038e5
📒 Files selected for processing (34)
crates/fgumi-bgzf/src/reader.rscrates/fgumi-bgzf/src/writer.rscrates/fgumi-consensus/src/base_builder.rscrates/fgumi-consensus/src/caller.rscrates/fgumi-consensus/src/codec_caller.rscrates/fgumi-consensus/src/duplex_caller.rscrates/fgumi-consensus/src/filter.rscrates/fgumi-consensus/src/overlapping.rscrates/fgumi-consensus/src/vanilla_caller.rscrates/fgumi-consensus/src/vendored/bam_codec/encoder/mod.rscrates/fgumi-consensus/src/vendored/bam_codec/encoder/quality_scores.rscrates/fgumi-consensus/src/vendored/bam_codec/encoder/reference_sequence_id.rscrates/fgumi-consensus/src/vendored/bam_codec/encoder/sequence.rscrates/fgumi-dna/src/bitenc.rscrates/fgumi-metrics/src/consensus.rscrates/fgumi-metrics/src/duplex.rscrates/fgumi-metrics/src/shared.rscrates/fgumi-metrics/src/simplex.rscrates/fgumi-raw-bam/src/builder.rscrates/fgumi-raw-bam/src/fields.rscrates/fgumi-raw-bam/src/noodles_compat.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/src/sequence.rscrates/fgumi-raw-bam/src/tags.rscrates/fgumi-raw-bam/src/testutil.rscrates/fgumi-sam/src/alignment_tags.rscrates/fgumi-sam/src/builder.rscrates/fgumi-sam/src/clipper.rscrates/fgumi-sam/src/lib.rscrates/fgumi-simd-fastq/benches/fastq_parsing.rscrates/fgumi-simd-fastq/src/parser.rscrates/fgumi-simd-fastq/src/reader.rscrates/fgumi-umi/src/assigner.rscrates/fgumi-umi/src/lib.rs
- Fix terse/inconsistent expect messages in RecordPairBuilder::build() to match the descriptive style used elsewhere in builder.rs - Replace generic "NonZero::new should succeed" messages with specific value-based messages like "8 is non-zero" for consistency - Fix misleading "expected item at index" on HashMap::get() calls to say "key should be present in groups" - Replace cryptic "failed to get sr" messages with "source read should be Some" across vanilla_caller.rs test code - Fix ambiguous "source should be Some/Ok" to "source should be Some"
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
unwrap()calls across 34 files incrates/expect()with invariant-documenting messagesexpect()with context-specific failure messagesunwrap()calls remain, all in doc comments (standard Rust convention)Test plan
cargo ci-test— all 2,213 tests passcargo ci-fmt— formatting cleancargo ci-lint— no warnings