Skip to content

feat(sort,merge): add --cell-tag flag to override default CB - #358

Closed
nh13 wants to merge 1 commit into
mainfrom
nh/sort-cell-tag-arg
Closed

nh13 wants to merge 1 commit into
mainfrom
nh/sort-cell-tag-arg

Conversation

@nh13

@nh13 nh13 commented May 18, 2026

Copy link
Copy Markdown
Member

Summary

When sorting or merging with --order template-coordinate, fgumi previously hard-coded SamTag::CB as the cell-barcode tag, with no way to use a different two-character SAM tag (e.g. CR for raw cell barcodes, or a vendor-specific tag).

Add an optional --cell-tag <XX> flag to both fgumi sort and fgumi merge:

  • When omitted, behavior is unchanged (defaults to CB).
  • When provided, the value must be a valid two-character SAM tag ([A-Za-z][A-Za-z0-9]), parsed via SamTag::FromStr.
  • Passing --cell-tag with any non-template-coordinate sort order is a hard error (--cell-tag is only valid with --order template-coordinate).

Mirrors samtools' equivalent CLI shape for cell-aware template-coordinate sorting.

This was flagged by the Opus review of #356 (issue #352) as a known limitation; filing as a standalone enhancement since it's a real Sort/Merge feature, not test mechanics.

Test plan

  • Two new unit tests in sort.rs: test_parse_cell_tag_with_override, test_parse_cell_tag_override_rejected_for_non_template_coordinate
  • Existing parameterized test_parse_cell_tag cases still pass (CB default preserved)
  • cargo nextest run --features compare,simulate — 2105/2105 passed, 23 skipped
  • cargo ci-fmt clean
  • cargo ci-lint clean

Backward compatibility

No CLI surface removed; new flag is optional with the prior hard-coded default preserved.

@nh13
nh13 temporarily deployed to github-actions May 18, 2026 22:07 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented May 18, 2026 •

Copy link
Copy Markdown

Review Change Stack

Warning

Rate limit exceeded

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

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ 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: 785b8c5e-931c-494c-8bfa-bfe49e1de9e2

📥 Commits

Reviewing files that changed from the base of the PR and between 77a9fc7 and 71aa612.

📒 Files selected for processing (2)
  • src/lib/commands/merge.rs
  • src/lib/commands/sort.rs
📝 Walkthrough

Walkthrough

The PR introduces a --cell-tag CLI option to the merge and sort commands, enabling users to override the default SAM tag used for template-coordinate operations. In sort.rs, the parse_cell_tag helper is updated to accept an optional override, returning the user-supplied tag for template-coordinate or defaulting to CB, and rejecting the override for other sort orders. The merge command mirrors this pattern: it adds the same CLI field and wires it into the execute method's call to parse_cell_tag. Both commands' unit tests are updated to initialize the new field and validate the override behavior.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: adding a --cell-tag flag to both sort and merge commands to override the default CB tag.
Description check ✅ Passed The description directly explains the changes: adds optional --cell-tag flag to sort/merge, preserves CB default, validates tag format, and enforces template-coordinate-only usage.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ 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/sort-cell-tag-arg

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented May 18, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 90.94%. Comparing base (c3b6dc6) to head (71aa612).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #358   +/-   ##
=======================================
  Coverage   90.93%   90.94%           
=======================================
  Files          78       78           
  Lines       51046    51072   +26     
=======================================
+ Hits        46421    46447   +26     
  Misses       4625     4625           

☔ 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 commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai resume

@nh13

nh13 commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
✅ Actions performed

Reviews resumed.

@coderabbitai

coderabbitai Bot commented May 20, 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 May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai resume

@nh13

nh13 commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
✅ Actions performed

Reviews resumed.

@coderabbitai

coderabbitai Bot commented May 20, 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 May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai resume

@nh13

nh13 commented May 20, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented May 20, 2026

Copy link
Copy Markdown
✅ Actions performed

Reviews resumed.

@coderabbitai

coderabbitai Bot commented May 20, 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.

When sorting or merging with --order template-coordinate, fgumi
previously hard-coded SamTag::CB as the cell-barcode tag, with no
way to use a different two-character SAM tag (e.g. CR for raw cell
barcodes, or a vendor-specific tag).

Add an optional --cell-tag <XX> flag to both `fgumi sort` and
`fgumi merge`. When omitted, behavior is unchanged (CB default).
When provided, the value must be a valid two-character SAM tag and
parses via SamTag::FromStr. Passing --cell-tag with any non-
template-coordinate sort order is a hard error.

Mirrors samtools' `--barcode-tag` for `samtools sort --template-
coordinate`.
@nh13
nh13 force-pushed the nh/sort-cell-tag-arg branch from 77a9fc7 to 71aa612 Compare May 21, 2026 23:21
@nh13
nh13 temporarily deployed to github-actions May 21, 2026 23:21 — with GitHub Actions Inactive
@nh13

nh13 commented May 22, 2026

Copy link
Copy Markdown
Member Author

Closing as won't fix.

After reflection, we want fgumi to be opinionated that the cell barcode tag is CB, per the SAM spec. This matches fgbio's TemplateCoordinate sort, which also reads SAMTag.CB unconditionally and exposes no override. Keeping the tag fixed avoids a configuration knob whose only legitimate uses (raw CR, vendor-specific tags) are better solved by re-tagging upstream of sort/merge than by parameterizing every downstream tool.

Note for the record: this PR was motivated by an AI review comment on #356, not a user request — no one has asked for a non-CB cell tag in practice. If that changes, we can revisit.

@nh13 nh13 closed this May 22, 2026
@nh13
nh13 deleted the nh/sort-cell-tag-arg branch May 22, 2026 00:29

This branch was previously deployed

1 inactive deployment
github-actions — 71aa6126 Deployed May 21, 2026 by nh13 via coverage #1489
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant