Skip to content

refactor(zipper): use RawRecord methods in restore_unconverted_bases_in_raw_record - #286

Merged
nh13 merged 1 commit into
mainfrom
nh/raw-bam-api-6-zipper-raw-record-methods
Apr 18, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/raw-bam-api-6-zipper-raw-record-methods

Conversation

@nh13

@nh13 nh13 commented Apr 17, 2026

Copy link
Copy Markdown
Member

Summary

Cosmetic refactor of `restore_unconverted_bases_in_raw_record` (added in #274) to use `RawRecord`'s inherent methods instead of `bam_fields::*` free-function calls + `&mut Vec` plumbing. Same behavior, same byte-for-byte output, same 17 tests pass.

Part 1 of 7 in the follow-up series closing #272.

Changes

Signature tightens from `record: &mut Vec` to `rec: &mut RawRecord`, which reads more clearly at the call site. Internal replacements:

  • `bam_fields::flags(record)` → `rec.is_unmapped()` / `rec.is_reverse()`
  • `bam_fields::ref_id` / `pos` → `rec.ref_id()` / `rec.pos()`
  • `bam_fields::find_string_tag_in_record` → `rec.tags().find_string()`
  • `bam_fields::get_cigar_ops` → `rec.cigar_ops_vec()`
  • `bam_fields::reference_length_from_cigar` → `rec.reference_length()`
  • `bam_fields::get_base` / `set_base` → `rec.get_base()` / `rec.set_base()` (seq_offset no longer needed at call site)
  • `bam_fields::remove_tag` → `rec.tags_editor().remove()`

The single caller (`restore_unconverted_bases_in_raw_template`) bridges `template.raw_records` (still `Vec<Vec>` in `Template`) by wrapping each entry in `RawRecord::from` via `std::mem::take` and restoring it with `into_inner`.

Test plan

  • `cargo ci-test` — 2541/2541 pass
  • `cargo ci-fmt` clean
  • `cargo ci-lint` clean (clippy pedantic + `-D warnings`)
  • All 17 `test_restore_unconverted_bases_*` tests (RecordBuf path + raw-byte path + parsing) pass unchanged

Stack

  • PR 1 (this): zipper `restore_unconverted_bases_in_raw_record` → RawRecord methods
  • PR 2: fgumi-raw-bam additive API (write helpers, CigarOp/Kind, TagValue, converters, SamBuilder, IndexedRawBamReader, inherent delegators)
  • PR 3: fgumi-sam raw helpers + RawRecordClipper
  • PR 4: consensus internals → RawRecord + drop RecordBuf variants
  • PR 5: commands + storage + pipeline collapses
  • PR 6: workspace test migrations to SamBuilder
  • PR 7: delete vendored bam_codec + simulate → SamBuilder

@nh13 nh13 added the raw-bam label Apr 17, 2026
@nh13
nh13 temporarily deployed to github-actions April 17, 2026 20:06 — with GitHub Actions Inactive
@codecov

codecov Bot commented Apr 17, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 84.00000% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.03%. Comparing base (05e89b8) to head (f820ecd).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/commands/zipper.rs 84.00% 4 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #286   +/-   ##
=======================================
  Coverage   90.03%   90.03%           
=======================================
  Files         122      122           
  Lines       61791    61813   +22     
=======================================
+ Hits        55632    55652   +20     
- Misses       6159     6161    +2     

☔ 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.

@nh13
nh13 marked this pull request as ready for review April 17, 2026 22:28
@coderabbitai

coderabbitai Bot commented Apr 17, 2026 •

Copy link
Copy Markdown

Warning

Rate limit exceeded

@nh13 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 22 minutes and 35 seconds before requesting another review.

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 22 minutes and 35 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 82044aaa-ece1-4bff-8450-eb79d99ed4af

📥 Commits

Reviewing files that changed from the base of the PR and between 7a3a042 and f820ecd.

📒 Files selected for processing (1)
  • src/lib/commands/zipper.rs
📝 Walkthrough

Walkthrough

The methylation restore function in zipper.rs was refactored to use RawRecord from fgumi_raw_bam instead of directly mutating Vec<u8> buffers. Raw BAM byte accessors and helpers were replaced with RawRecord methods for reading/writing bases, checking unmapped/reverse status, looking up tags, and managing CIGAR operations. Tags like NM and MD are conditionally removed only when the sequence is modified. Tests were updated to construct RawRecord instances from encoded bytes and convert back afterward, maintaining byte-level parity in test verification.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly and specifically summarizes the main refactoring: replacing bam_fields calls with RawRecord methods in the restore_unconverted_bases_in_raw_record function.
Description check ✅ Passed Description is directly relevant, detailing the cosmetic refactor with clear mapping of old calls to new RawRecord methods, test results, and context within a larger PR series.
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 unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nh/raw-bam-api-6-zipper-raw-record-methods

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.

@nh13

nh13 commented Apr 18, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 18, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

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.

@nh13

nh13 commented Apr 18, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 18, 2026

Copy link
Copy Markdown
✅ Actions performed

Review triggered.

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.

…in_raw_record

Swap the bam_fields::* free-function calls + manual &mut Vec<u8> plumbing
for RawRecord's inherent methods. Cosmetic-only: same behavior, same
byte-for-byte output, same 17 tests pass. Function signature tightens
from `record: &mut Vec<u8>` to `rec: &mut RawRecord`, which reads more
clearly at the call site.

Replaces:
- bam_fields::flags(record)             -> rec.is_unmapped() / rec.is_reverse()
- bam_fields::ref_id / pos              -> rec.ref_id() / rec.pos()
- bam_fields::find_string_tag_in_record -> rec.tags().find_string()
- bam_fields::get_cigar_ops             -> rec.cigar_ops_vec()
- bam_fields::reference_length_from_cigar -> rec.reference_length()
- bam_fields::l_seq / seq_offset        -> rec.l_seq(); seq_off no longer
                                           needed (get_base/set_base compute
                                           internally)
- bam_fields::get_base / set_base       -> rec.get_base() / rec.set_base()
- bam_fields::remove_tag                -> rec.tags_editor().remove()

The single caller (restore_unconverted_bases_in_raw_template) bridges
template.raw_records (still Vec<Vec<u8>> in Template) by wrapping each
entry in RawRecord::from via std::mem::take and restoring it with
into_inner.
@nh13
nh13 force-pushed the nh/raw-bam-api-6-zipper-raw-record-methods branch from 7a3a042 to f820ecd Compare April 18, 2026 07:36
@nh13
nh13 temporarily deployed to github-actions April 18, 2026 07:36 — with GitHub Actions Inactive
@nh13
nh13 merged commit 3a5650f into main Apr 18, 2026
7 of 9 checks passed
@nh13
nh13 deleted the nh/raw-bam-api-6-zipper-raw-record-methods branch April 18, 2026 07:39
This was referenced Apr 18, 2026

This branch was previously deployed

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant