Skip to content

docs(sort): keep the binary name out of per-argument help - #670

Merged
nh13 merged 1 commit into
mainfrom
nh/sort-threads-help-binary-agnostic
Jul 29, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/sort-threads-help-binary-agnostic

Conversation

@nh13

@nh13 nh13 commented Jul 28, 2026 •

Copy link
Copy Markdown
Member

Sort is designed to be pulled into other binaries with #[command(flatten)], which means its per-argument help renders under a program name that is not fgumi. So a concrete fgumi ... invocation in per-argument help tells those users to run a command they do not have.

--sort-threads (added in #608) was doing exactly that:

Defaults to `--threads`. Lower this to cede cores to an upstream producer while keeping
the merge wide -- e.g. in `bwa mem -t 32 ... | fgumi sort -@ 8 --sort-threads 4`, ingest
competes with the aligner but the merge does not.

It was also the only per-argument doc in Sort carrying a concrete invocation — every other one already lives in the command-level EXAMPLES block. So this is a consistency fix rather than a special case.

What changed

The pipeline example moves to EXAMPLES, alongside the other twelve invocations:

  # Cede cores to the aligner during ingest, but keep the merge wide
  bwa mem -t 32 ref.fa r1.fq r2.fq | fgumi sort -i - -o sorted.bam -@ 8 --sort-threads 4

The flag help now describes the flag, and states the mechanism the old text left implicit (the merge does not):

Defaults to `--threads`. Lower this to cede cores to an upstream producer while keeping
the merge wide -- with `-@ 8 --sort-threads 4`, ingest contends with the producer over
only 4 threads, while the merge still uses 8 because it cannot start until the input is
exhausted, by which point the producer has finished writing.

That rationale is checked against the engine, not assumed: RawExternalSorter::enter_output_phase is called "exactly once, immediately after ingest completes," so it holds on every sort path, not just the spilling one.

--order had a milder version of the same thing — `queryname::lexicographical` Alias; fgumi emits `queryname:lexicographical` in @HD SS — now written passively (Alias; written as ...). Same information, and it stays true of any binary embedding the engine.

Regression guard

test_arg_help_does_not_name_the_binary walks every argument's short and long help and rejects the binary name, so this cannot creep back in. It asserts a non-trivial arg count first, so an empty walk cannot pass vacuously.

I confirmed the test has teeth by temporarily reintroducing fgumi sort into the --sort-threads help: it fails and names the offending flag.

help for `--sort_threads` names the `fgumi` binary; describe the flag instead and put
worked invocations in the command-level EXAMPLES block. Help was: ...

What is deliberately left alone

The command-level long_about still contains many fgumi sort invocations, and the test does not walk it. A wrapper replaces long_about wholesale, so it never reaches a downstream --help — which makes it the correct home for worked invocations. The exemption is documented on the test so the asymmetry reads as a decision rather than an oversight.

Verification

cargo ci-fmt      # clean
cargo ci-lint     # clean (workspace, all-targets, -D warnings -W clippy::pedantic)
cargo ci-test     # 6881 tests run: 6881 passed, 27 skipped
cargo ci-doctest  # clean

Docs-only change to user-facing help text plus one new test; no behavior change. grep -rn sort-threads docs/ is empty, so there is no mdbook copy of this text to keep in sync.

Motivation

Surfaced while bumping the downstream mako sorter (a thin Sort-flattening wrapper) to fgumi 0.5.0: mako --help began advertising a fgumi sort invocation. Before this change its --help contained one such invocation; after, none — every remaining mention is in long_about, which mako replaces.

Summary by CodeRabbit

  • Documentation

    • Refreshed sort command help text with clearer examples and improved wording for --order and --sort-threads options.
  • Tests

    • Added a new test to verify that per-argument help output does not include the command name, ensuring cleaner and more consistent help text presentation.

@nh13
nh13 temporarily deployed to github-actions July 28, 2026 23:29 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •

Copy link
Copy Markdown

Review Change Stack

Note

Reviews paused

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f3e35940-99bb-48f1-94d3-c8694223e92a

📥 Commits

Reviewing files that changed from the base of the PR and between 3840bf9 and b16ace6.

📒 Files selected for processing (1)
  • src/lib/commands/sort.rs

Walkthrough

The fgumi sort help examples and option descriptions were revised. A unit test now checks that individual clap argument help strings do not include the fgumi binary name.

Changes

Sort command help

Layer / File(s) Summary
Update and validate sort help
src/lib/commands/sort.rs
The pipeline example, queryname sub-sort alias description, and thread scheduling wording were revised. Tests cover short and long argument help for binary-name references.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: tfenne

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the doc-only change to remove the binary name from per-argument help.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nh/sort-threads-help-binary-agnostic

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

@codecov

codecov Bot commented Jul 28, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.33333% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 93.94%. Comparing base (3df1022) to head (b16ace6).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/commands/sort.rs 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #670      +/-   ##
==========================================
- Coverage   93.96%   93.94%   -0.03%     
==========================================
  Files         178      178              
  Lines      108043   108058      +15     
==========================================
- Hits       101528   101517      -11     
- Misses       6515     6541      +26     

☔ View full report in Codecov by Harness.
📢 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 Jul 28, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

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 Jul 28, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Jul 28, 2026 •

Copy link
Copy Markdown
✅ Action performed

Full review finished.


Your included review limit is currently reached under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 57 minutes.

@nh13

nh13 commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

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 Jul 29, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

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 Jul 29, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

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.

@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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/commands/sort.rs`:
- Around line 741-744: Update the assertion in the per-argument help test to
detect `fgumi` as a standalone token using token-boundary matching, including
when followed by punctuation or end-of-string. Preserve the existing failure
message and ensure binary-name occurrences embedded within larger words are not
rejected.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: ba5c4140-6862-4157-83ac-f758f942184a

📥 Commits

Reviewing files that changed from the base of the PR and between 3df1022 and 3840bf9.

📒 Files selected for processing (1)
  • src/lib/commands/sort.rs

Comment thread src/lib/commands/sort.rs
@nh13

nh13 commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai pause

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
✅ Action performed

Reviews paused.

`Sort` is pulled into other binaries with `#[command(flatten)]`, so its
per-argument help renders under a program name that is not `fgumi`. The
`--sort-threads` help embedded a worked
`bwa mem -t 32 ... | fgumi sort -@ 8 --sort-threads 4` pipeline, which
tells those users to run a command they do not have. It was also the only
per-argument doc carrying a concrete invocation; every other one already
lives in the command-level EXAMPLES block.

Move the pipeline to EXAMPLES and leave the flag help describing the
flag. The replacement also states why the merge can stay wide -- it
cannot start until the input is exhausted, by which point the producer
has finished writing -- which the previous "the merge does not" left
implicit. `enter_output_phase` is called exactly once, immediately after
ingest completes, so that holds for every sort path.

`--order` had a milder version of the same thing ("fgumi emits
`queryname:lexicographical` in @hd SS"); it is now written passively,
which stays true of any binary embedding the engine.

A new test walks every argument's help and rejects the binary name so
this cannot creep back. The match is on identifier-shaped tokens rather
than the literal `"fgumi "`, so a trailing, backticked, or punctuated
mention is caught too, while `fgumidocs` and `fgumi_sort` are not.
Command-level `long_about` is deliberately exempt and is not walked: a
wrapper replaces it wholesale, so it remains the right home for worked
`fgumi sort` invocations.
@nh13
nh13 force-pushed the nh/sort-threads-help-binary-agnostic branch from 3840bf9 to b16ace6 Compare July 29, 2026 04:33
@nh13
nh13 temporarily deployed to github-actions July 29, 2026 04:33 — with GitHub Actions Inactive
@nh13

nh13 commented Jul 29, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026 •

Copy link
Copy Markdown
✅ Action performed

Review finished.

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 merged commit 3d27019 into main Jul 29, 2026
14 checks passed
@nh13
nh13 deleted the nh/sort-threads-help-binary-agnostic branch July 29, 2026 11:32
@nh13 nh13 mentioned this pull request Aug 15, 2026

This branch was previously deployed

1 inactive deployment
github-actions — b16ace6d Deployed Jul 29, 2026 by nh13 via coverage #3155
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