fix(zipper): name the tag-skipping flag after the tag it actually writes - #617
Merged
Merged
Conversation
The flag was `--skip-pa-tags` and its help said it skipped "`pa` (primary alignment) tags", but the field is `skip_tc_tags` and the command writes `tc` (template coordinate). No `pa` tag has ever been written, so a user grepping the output for what the flag named found nothing. Renames it to `--skip-tc-tags` and keeps `--skip-pa-tags` as an alias rather than renaming outright, so existing scripts keep working; repo precedent for that is the `--queue-memory*` aliases. The help text and the internal step comment now say `tc`. Note the tag bytes are lowercase `tc` -- `SamTag::TC` is the constant's name, not the tag's spelling -- so the help refers to it that way. The existing parsing test gains cases for the new spelling alongside the alias, covering the bare flag, explicit true/false, and the `=` form for both.
|
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 (1)
WalkthroughThe Zipper flag is renamed to ChangesZipper template-coordinate tag flag
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #617 +/- ##
==========================================
- Coverage 93.42% 93.42% -0.01%
==========================================
Files 175 175
Lines 104807 104797 -10
==========================================
- Hits 97919 97909 -10
Misses 6888 6888 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Member
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Merged
nh13
added a commit
that referenced
this pull request
Aug 1, 2026
`--skip-tc-tags` long help said the former `--skip-pa-tags` spelling "described a `pa` tag that this command has never written ... the tag is and always was `tc`". Both halves are false: v0.1.3 zipper contains `let pa_tag = b"pa"`, and 40b685c ("rename sort-key tag from pa to tc", 0.2.0) is the commit that changed the literal to `tc`. The claim came in with #617, which correctly renamed the flag but overcorrected on the history. It also misinforms the users who have such BAMs. A BAM zippered by a pre-0.2.0 release carries `pa` and no `tc`; dedup counts `missing_tc_tag` on its secondary/supplementary reads and bails with a re-zipper instruction. The help told those users nothing about their output had changed. Replace the sentence with the actual history and the way out, and correct the 0.2.0 changelog entry, which claimed the flag rename landed there. It renamed the tag and the `missing_pa_tag` metric; the flag stayed spelled `--skip-pa-tags` through 0.4.0 and was renamed in 0.5.0 (#617). Anyone who followed those release notes and passed `--skip-tc-tags` on 0.2.0-0.4.0 got an unknown-argument error. Two tests pin the help: one rejects a denial of the `pa` history, one requires the version boundary, `re-zipper`, and `dedup` so the help keeps pointing at the failure a user with pre-0.2.0 output will actually hit. Closes #679
nh13
added a commit
that referenced
this pull request
Aug 2, 2026
`--skip-tc-tags` long help said the former `--skip-pa-tags` spelling "described a `pa` tag that this command has never written ... the tag is and always was `tc`". Both halves are false: v0.1.3 zipper contains `let pa_tag = b"pa"`, and 40b685c ("rename sort-key tag from pa to tc", 0.2.0) is the commit that changed the literal to `tc`. The claim came in with #617, which correctly renamed the flag but overcorrected on the history. It also misinforms the users who have such BAMs. A BAM zippered by a pre-0.2.0 release carries `pa` and no `tc`; dedup counts `missing_tc_tag` on its secondary/supplementary reads and bails with a re-zipper instruction. The help told those users nothing about their output had changed. Replace the sentence with the actual history and the way out, and correct the 0.2.0 changelog entry, which claimed the flag rename landed there. It renamed the tag and the `missing_pa_tag` metric; the flag stayed spelled `--skip-pa-tags` through 0.4.0 and was renamed in 0.5.0 (#617). Anyone who followed those release notes and passed `--skip-tc-tags` on 0.2.0-0.4.0 got an unknown-argument error. Two tests pin the help: one rejects a denial of the `pa` history, one requires the version boundary, `re-zipper`, and `dedup` so the help keeps pointing at the failure a user with pre-0.2.0 output will actually hit. Closes #679
nh13
added a commit
that referenced
this pull request
Aug 2, 2026
`--skip-tc-tags` long help said the former `--skip-pa-tags` spelling "described a `pa` tag that this command has never written ... the tag is and always was `tc`". Both halves are false: v0.1.3 zipper contains `let pa_tag = b"pa"`, and 40b685c ("rename sort-key tag from pa to tc", 0.2.0) is the commit that changed the literal to `tc`. The claim came in with #617, which correctly renamed the flag but overcorrected on the history. It also misinforms the users who have such BAMs. A BAM zippered by a pre-0.2.0 release carries `pa` and no `tc`; dedup counts `missing_tc_tag` on its secondary/supplementary reads and bails with a re-zipper instruction. The help told those users nothing about their output had changed. Replace the sentence with the actual history and the way out, and correct the 0.2.0 changelog entry, which claimed the flag rename landed there. It renamed the tag and the `missing_pa_tag` metric; the flag stayed spelled `--skip-pa-tags` through 0.4.0 and was renamed in 0.5.0 (#617). Anyone who followed those release notes and passed `--skip-tc-tags` on 0.2.0-0.4.0 got an unknown-argument error. Scope the rename-in-place escape hatch in both the help and the changelog. `pa` is also bwa-mem's `pa:f` score tag on primary reads, so an unqualified "rename the tag" points at the wrong one: only the legacy sort key itself — the six-element `pa:B:i` array on secondary and supplementary reads — may be renamed to `tc`. Renaming `pa:f` destroys alignment metadata and leaves a `tc:f` where a `tc:B:i` belongs, and that fails silently: dedup only checks that `tc` is present, so a wrong-typed tag clears `missing_tc_tag` and then fails to parse in `TemplateCoordinateInfo::from_tag_value`. An rstest table pins each clause of that history and every documented way out — including the two cases that scope the rename — so the help keeps pointing at the failure a user with pre-0.2.0 output will actually hit; a companion test rejects any return to denying the `pa` history. Closes #679
nh13
added a commit
that referenced
this pull request
Aug 3, 2026
`--skip-tc-tags` long help said the former `--skip-pa-tags` spelling "described a `pa` tag that this command has never written ... the tag is and always was `tc`". Both halves are false: v0.1.3 zipper contains `let pa_tag = b"pa"`, and 40b685c ("rename sort-key tag from pa to tc", 0.2.0) is the commit that changed the literal to `tc`. The claim came in with #617, which correctly renamed the flag but overcorrected on the history. It also misinforms the users who have such BAMs. A BAM zippered by a pre-0.2.0 release carries `pa` and no `tc`; dedup counts `missing_tc_tag` on its secondary/supplementary reads and bails with a re-zipper instruction. The help told those users nothing about their output had changed. Replace the sentence with the actual history and the way out, and correct the 0.2.0 changelog entry, which claimed the flag rename landed there. It renamed the tag and the `missing_pa_tag` metric; the flag stayed spelled `--skip-pa-tags` through 0.4.0 and was renamed in 0.5.0 (#617). Anyone who followed those release notes and passed `--skip-tc-tags` on 0.2.0-0.4.0 got an unknown-argument error. Scope the rename-in-place escape hatch in both the help and the changelog. `pa` is also bwa-mem's `pa:f` score tag on primary reads, so an unqualified "rename the tag" points at the wrong one: only the legacy sort key itself — the six-element `pa:B:i` array on secondary and supplementary reads — may be renamed to `tc`. Renaming `pa:f` destroys alignment metadata and leaves a `tc:f` where a `tc:B:i` belongs, and that fails silently: dedup only checks that `tc` is present, so a wrong-typed tag clears `missing_tc_tag` and then fails to parse in `TemplateCoordinateInfo::from_tag_value`. An rstest table pins each clause of that history and every documented way out — including the two cases that scope the rename — so the help keeps pointing at the failure a user with pre-0.2.0 output will actually hit; a companion test rejects any return to denying the `pa` history. Closes #679
nh13
added a commit
that referenced
this pull request
Aug 4, 2026
`--skip-tc-tags` long help said the former `--skip-pa-tags` spelling "described a `pa` tag that this command has never written ... the tag is and always was `tc`". Both halves are false: v0.1.3 zipper contains `let pa_tag = b"pa"`, and 40b685c ("rename sort-key tag from pa to tc", 0.2.0) is the commit that changed the literal to `tc`. The claim came in with #617, which correctly renamed the flag but overcorrected on the history. It also misinforms the users who have such BAMs. A BAM zippered by a pre-0.2.0 release carries `pa` and no `tc`; dedup counts `missing_tc_tag` on its secondary/supplementary reads and bails with a re-zipper instruction. The help told those users nothing about their output had changed. Replace the sentence with the actual history and the way out, and correct the 0.2.0 changelog entry, which claimed the flag rename landed there. It renamed the tag and the `missing_pa_tag` metric; the flag stayed spelled `--skip-pa-tags` through 0.4.0 and was renamed in 0.5.0 (#617). Anyone who followed those release notes and passed `--skip-tc-tags` on 0.2.0-0.4.0 got an unknown-argument error. Scope the rename-in-place escape hatch in both the help and the changelog. `pa` is also bwa-mem's `pa:f` score tag on primary reads, so an unqualified "rename the tag" points at the wrong one: only the legacy sort key itself — the six-element `pa:B:i` array on secondary and supplementary reads — may be renamed to `tc`. Renaming `pa:f` destroys alignment metadata and leaves a `tc:f` where a `tc:B:i` belongs, and that fails silently: dedup only checks that `tc` is present, so a wrong-typed tag clears `missing_tc_tag` and then fails to parse in `TemplateCoordinateInfo::from_tag_value`. An rstest table pins each clause of that history and every documented way out — including the two cases that scope the rename — so the help keeps pointing at the failure a user with pre-0.2.0 output will actually hit; a companion test rejects any return to denying the `pa` history. Closes #679
nh13
added a commit
that referenced
this pull request
Aug 4, 2026
`--skip-tc-tags` long help said the former `--skip-pa-tags` spelling "described a `pa` tag that this command has never written ... the tag is and always was `tc`". Both halves are false: v0.1.3 zipper contains `let pa_tag = b"pa"`, and 40b685c ("rename sort-key tag from pa to tc", 0.2.0) is the commit that changed the literal to `tc`. The claim came in with #617, which correctly renamed the flag but overcorrected on the history. It also misinforms the users who have such BAMs. A BAM zippered by a pre-0.2.0 release carries `pa` and no `tc`; dedup counts `missing_tc_tag` on its secondary/supplementary reads and bails with a re-zipper instruction. The help told those users nothing about their output had changed. Replace the sentence with the actual history and the way out, and correct the 0.2.0 changelog entry, which claimed the flag rename landed there. It renamed the tag and the `missing_pa_tag` metric; the flag stayed spelled `--skip-pa-tags` through 0.4.0 and was renamed in 0.5.0 (#617). Anyone who followed those release notes and passed `--skip-tc-tags` on 0.2.0-0.4.0 got an unknown-argument error. Scope the rename-in-place escape hatch in both the help and the changelog. `pa` is also bwa-mem's `pa:f` score tag on primary reads, so an unqualified "rename the tag" points at the wrong one: only the legacy sort key itself — the six-element `pa:B:i` array on secondary and supplementary reads — may be renamed to `tc`. Renaming `pa:f` destroys alignment metadata and leaves a `tc:f` where a `tc:B:i` belongs, and that fails silently: dedup only checks that `tc` is present, so a wrong-typed tag clears `missing_tc_tag` and then fails to parse in `TemplateCoordinateInfo::from_tag_value`. An rstest table pins each clause of that history and every documented way out — including the two cases that scope the rename — so the help keeps pointing at the failure a user with pre-0.2.0 output will actually hit; a companion test rejects any return to denying the `pa` history. Closes #679
This was referenced Aug 5, 2026
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.
The flag was
--skip-pa-tagsand its help said it skipped "pa(primary alignment) tags" — but the struct field isskip_tc_tagsand the command writestc(template coordinate). Nopatag has ever been written, so a user who grepped the output for the thing the flag named found nothing.Renamed to
--skip-tc-tags, with--skip-pa-tagskept as an alias rather than renamed outright, so existing scripts keep working. Repo precedent for that is the--queue-memory*aliases. Help text and the internal step comment now saytc.One detail worth flagging: the tag bytes are lowercase
tc, notTC—SamTag::TCis the Rust constant's name, not the tag's spelling (assert_eq!(*SamTag::TC, [b't', b'c'])). The help refers to it astcaccordingly, since that is what a user would actually grep for.The existing parsing test gains cases for the new spelling alongside the retained alias, covering the bare flag, explicit
true/false, and the=form for both — 13 cases total.cargo ci-test(5539 tests),cargo ci-fmt,cargo ci-lint,RUSTDOCFLAGS="-D warnings" cargo ci-docpass.Summary by CodeRabbit
New Features
--skip-tc-tagscommand-line option for controlling template-coordinate tags on supplementary reads.--skip-pa-tagsas a backward-compatible alias.Documentation