refactor(sam): remove dead SamRecordClipper, port its tests to RawRecordClipper - #854
Conversation
…ordClipper `SamRecordClipper` (the RecordBuf-based clipper) had no production caller left -- `fgumi clip` and every consensus caller use the raw-byte `RawRecordClipper`, and the only `SamRecordClipper::new`/`with_auto_clip` calls in the workspace were in its own tests. It was the tail of the RecordBuf -> raw-byte migration. Remove the struct, its impl, and the dead helpers it alone used (`reorient_strand_tags`, the `array_len`/`slice_array` macros), and drop it from the `fgumi-sam` public API. Its ~110 behavioral tests were the bulk of the clipping coverage; `RawRecordClipper` was largely verified only by cross-checking against it. To avoid losing that coverage, port every test to `RawRecordClipper` via a `RawClipperOnBuf` test adapter that runs the raw clipper through the RecordBuf API (RecordBuf -> raw -> clip -> RecordBuf), so each test's setup and assertions run verbatim against the real raw clipper. The two non-UTF-8 tag tests -- which a noodles RecordBuf cannot represent -- are rewritten to build the raw record and tag directly. The former cross-check module now verifies the buf<->raw round-trip preserves CIGAR/pos/seq/qual. The `create_test_record` helper now normalizes SEQ to the CIGAR's query length so the records are valid BAM for the raw encoder. Fix the doc references to the removed type in `RawRecordClipper` and `cigar.rs`. Closes #843.
|
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 (3)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe crate root no longer exports ChangesRaw clipper API cleanup
Estimated code review effort: 1 (Trivial) | ~2 minutes Merge Risk: ⚪ Minimal · up to This change removes an unused clipper implementation while preserving its behavioral coverage through the raw clipper; no actionable merge-blocking risk remains after normal checks and review. Suggested labels: 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #854 +/- ##
==========================================
+ Coverage 94.50% 94.58% +0.08%
==========================================
Files 190 193 +3
Lines 118135 119434 +1299
==========================================
+ Hits 111640 112967 +1327
+ Misses 6495 6467 -28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
Summary
SamRecordClipper(theRecordBuf-based clipper) had no production caller left —fgumi clipand every consensus caller use the raw-byteRawRecordClipper, and the onlySamRecordClipper::new/with_auto_clipcalls in the workspace were in its own tests. It was the tail of theRecordBuf→ raw-byte migration. This removes it (struct, impl, thearray_len/slice_arraymacros, and thereorient_strand_tagshelper it alone used) and drops it from thefgumi-sampublic API.Preserving coverage
Its ~110 behavioral tests were the bulk of the clipping coverage;
RawRecordClipperwas largely verified only by cross-checking against it. To avoid losing that coverage, every test is ported toRawRecordClippervia a test-onlyRawClipperOnBufadapter that runs the raw clipper through theRecordBufAPI (RecordBuf→ raw → clip →RecordBuf), so each test's setup and assertions run verbatim against the real raw clipper. The two non-UTF-8 tag tests — which a noodlesRecordBufcannot represent — are rewritten to build the raw record and tag directly. The former cross-check module now verifies the buf↔raw round-trip preserves CIGAR/pos/seq/qual (both sides run the sameRawRecordClipper, so it no longer cross-checks an independent implementation; the concrete-expectation tests inmod testsare the correctness oracle). Thecreate_test_recordhelper now normalizes SEQ to the CIGAR's query length so the records are valid BAM for the raw encoder without changing any assertion.Reading order
The change is a large deletion (−1066 net); start with the
RawClipperOnBufadapter (newclip_test_adaptermodule) to see the porting mechanism, then thelib.rsexport removal, then the doc-reference fixes incrates/fgumi-raw-bam/src/cigar.rs.Full suite (154 clipper tests, all present), clippy, fmt, and rustdoc pass.
Closes #843.
Risk: command output none;
unsafechanges none, and CLAUDE.md allowlist changes none; memory bounds, queue capacity, and thread/backpressure policy changes none.Removes the unused
SamRecordClipperpublic re-export and updates documentation to use raw BAM clipping APIs.