Skip to content

refactor(raw-bam): deduplicate aux iteration, use field accessors - #135

Merged
nh13 merged 1 commit into
mainfrom
refactor/nh/simplify-fgumi-raw-bam
Feb 28, 2026
Merged

nh13 merged 1 commit into
mainfrom
refactor/nh/simplify-fgumi-raw-bam

Conversation

@nh13

@nh13 nh13 commented Feb 28, 2026

Copy link
Copy Markdown
Member

Summary

Seven simplification fixes reducing code by ~208 lines:

  • Consolidate base decode tables: Remove duplicate BASE_DECODE and DECODE constants, use single BAM_BASE_TO_ASCII everywhere
  • Deduplicate find_mc_tag: Rewrite as one-liner delegating to find_string_tag instead of duplicating the aux iteration loop
  • Use aux_data_slice in find_*_in_record: Replace manual offset computation in find_mc_tag_in_record and find_mi_tag_in_record
  • Use fields.rs accessors in sort.rs: Replace inline byte operations with named accessor functions (ref_id, pos, flags, read_name, etc.)
  • Unify compute_bases_past/before_ref_pos: Extract RefPosMode enum and shared compute_read_pos_at_ref implementation, eliminating copy-pasted overlap logic
  • Remove redundant assert + expect in builder.rs: Direct cast after length guard
  • Consolidate test aux-finders: Replace 143 lines of duplicated test helpers with 3-line delegations to production tags.rs functions

Test plan

  • cargo nextest run -p fgumi-raw-bam — all tests pass
  • cargo clippy -p fgumi-raw-bam --all-features -- -D warnings — no warnings
  • cargo check (full workspace) — clean
  • cargo ci-fmt — clean

@nh13
nh13 temporarily deployed to github-actions February 28, 2026 21:07 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Feb 28, 2026 •

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

This pull request refactors field access patterns across the BAM record handling crate by consolidating field extraction into helper functions, reducing code duplication, and removing redundant decoding tables. Changes include replacing manual byte offset calculations in sort.rs and tags.rs with accessor calls, removing BASE_DECODE in favor of existing BAM_BASE_TO_ASCII, introducing a RefPosMode enum for cleaner position calculation logic, and adjusting a u8 cast in builder.rs. Comprehensive unit test coverage is added across multiple modules (cigar.rs, fields.rs, sequence.rs, tags.rs) to exercise existing functionality. Public API signatures remain unchanged.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description check ✅ Passed Description clearly outlines all seven simplifications with specific examples matching the changeset across all modified files.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Title check ✅ Passed The title accurately describes the main objectives of the changeset: deduplicating aux iteration and consolidating field accessor usage throughout the crate.

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

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch refactor/nh/simplify-fgumi-raw-bam

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.

@codecov

codecov Bot commented Feb 28, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.49%. Comparing base (fa96485) to head (2ad9de6).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #135      +/-   ##
==========================================
- Coverage   83.50%   83.49%   -0.01%     
==========================================
  Files         126      126              
  Lines       51375    51375              
==========================================
- Hits        42901    42898       -3     
- Misses       8474     8477       +3     

☔ 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 added the enhancement New feature or request label Feb 28, 2026
@nh13
nh13 force-pushed the refactor/nh/simplify-fgumi-raw-bam branch from 1f7a048 to 1a35942 Compare February 28, 2026 22:56
@nh13
nh13 temporarily deployed to github-actions February 28, 2026 22:56 — with GitHub Actions Inactive

@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

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@crates/fgumi-raw-bam/src/cigar.rs`:
- Around line 868-990: Several test functions in this module have duplicate
names and will fail compilation (E0428); locate the duplicated test functions
named test_query_length_from_cigar_empty,
test_read_pos_at_ref_pos_raw_before_alignment,
test_read_pos_at_ref_pos_raw_in_deletion,
test_read_pos_at_ref_pos_raw_with_insertion, and
test_read_pos_at_ref_pos_raw_with_soft_clip and either remove the redundant
copies or rename them to unique identifiers (e.g., append _2 or a descriptive
suffix) so each test function name is unique; ensure any renamed tests keep the
same assertions and references to helper functions like query_length_from_cigar
and read_pos_at_ref_pos_raw.

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0488610 and 1a35942.

📒 Files selected for processing (8)
  • crates/fgumi-raw-bam/src/builder.rs
  • crates/fgumi-raw-bam/src/cigar.rs
  • crates/fgumi-raw-bam/src/fields.rs
  • crates/fgumi-raw-bam/src/overlap.rs
  • crates/fgumi-raw-bam/src/sequence.rs
  • crates/fgumi-raw-bam/src/sort.rs
  • crates/fgumi-raw-bam/src/tags.rs
  • crates/fgumi-raw-bam/src/testutil.rs

Comment thread crates/fgumi-raw-bam/src/cigar.rs
@nh13
nh13 force-pushed the refactor/nh/simplify-fgumi-raw-bam branch from 1a35942 to 2ad9de6 Compare February 28, 2026 23:10
@nh13
nh13 temporarily deployed to github-actions February 28, 2026 23:10 — with GitHub Actions Inactive
@nh13 nh13 changed the title Simplify fgumi-raw-bam: deduplicate aux iteration, use field accessors refactor(raw-bam): deduplicate aux iteration, use field accessors Feb 28, 2026
@nh13
nh13 merged commit 91b7749 into main Feb 28, 2026
6 of 7 checks passed
@nh13
nh13 deleted the refactor/nh/simplify-fgumi-raw-bam branch February 28, 2026 23:49
This was referenced Feb 28, 2026

This branch was previously deployed

1 inactive deployment
github-actions — 2ad9de68 Deployed Feb 28, 2026 by nh13 via coverage #484
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