Skip to content

refactor: replace all unwrap() calls in src/commands/ with expect() - #224

Merged
nh13 merged 2 commits into
mainfrom
nh/unwrap-audit-commands
Apr 4, 2026
Merged

nh13 merged 2 commits into
mainfrom
nh/unwrap-audit-commands

Conversation

@nh13

@nh13 nh13 commented Apr 4, 2026

Copy link
Copy Markdown
Member

Summary

  • Audit and replace all 527 unwrap() calls across 27 files in src/commands/
  • Production code (22 calls): converted to expect() with invariant-documenting messages explaining why the unwrap is safe
  • Test code (505 calls): converted to expect() with context-specific failure messages for better diagnostics
  • Zero unwrap() calls remain in src/commands/

Test plan

  • cargo ci-test — all 2,213 tests pass
  • cargo ci-fmt — formatting clean
  • cargo ci-lint — no warnings

@nh13
nh13 temporarily deployed to github-actions April 4, 2026 06:05 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Apr 4, 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 0 minutes and 22 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 0 minutes and 22 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: 9c80e7cd-4b06-4618-81bd-493b275da753

📥 Commits

Reviewing files that changed from the base of the PR and between 9abaf50 and a2ac65f.

📒 Files selected for processing (27)
  • src/commands/clip.rs
  • src/commands/codec.rs
  • src/commands/common.rs
  • src/commands/compare/bams.rs
  • src/commands/compare/raw_compare.rs
  • src/commands/consensus_runner.rs
  • src/commands/correct.rs
  • src/commands/dedup.rs
  • src/commands/downsample.rs
  • src/commands/duplex.rs
  • src/commands/duplex_metrics.rs
  • src/commands/extract.rs
  • src/commands/fastq.rs
  • src/commands/filter.rs
  • src/commands/group.rs
  • src/commands/merge.rs
  • src/commands/review.rs
  • src/commands/shared_metrics.rs
  • src/commands/simplex.rs
  • src/commands/simplex_metrics.rs
  • src/commands/simulate/common.rs
  • src/commands/simulate/correct_reads.rs
  • src/commands/simulate/fastq_reads.rs
  • src/commands/simulate/grouped_reads.rs
  • src/commands/simulate/mapped_reads.rs
  • src/commands/sort.rs
  • src/commands/zipper.rs
📝 Walkthrough

Walkthrough

This pull request systematically replaces .unwrap() calls with .expect(...) calls across numerous test files and a few production code paths. The changes affect approximately 25 files, adding explicit failure messages to result extraction operations on optional values, temporary file creation, BAM/FASTA file I/O, header construction, and tag parsing. No public API signatures or functional behavior are modified—only error handling and panic messaging are enhanced to aid debugging when tests fail or unexpected None values are encountered.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: a refactoring that replaces all unwrap() calls in src/commands/ with expect().
Description check ✅ Passed The description is directly related to the changeset, clearly summarizing the scope (527 calls across 27 files), breakdown (22 production, 505 test), and verification (all tests pass).
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/unwrap-audit-commands

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 added the hygiene label Apr 4, 2026
@codecov

codecov Bot commented Apr 4, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.63325% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.94%. Comparing base (0ee001f) to head (a2ac65f).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/commands/downsample.rs 90.47% 2 Missing ⚠️
src/commands/simulate/mapped_reads.rs 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #224      +/-   ##
==========================================
+ Coverage   88.88%   88.94%   +0.05%     
==========================================
  Files         113      113              
  Lines       53521    53807     +286     
==========================================
+ Hits        47574    47856     +282     
- Misses       5947     5951       +4     

☔ 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 temporarily deployed to github-actions April 4, 2026 07:05 — with GitHub Actions Inactive
@nh13

nh13 commented Apr 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 4, 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 4, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Apr 4, 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.

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

🧹 Nitpick comments (1)
src/commands/simulate/common.rs (1)

655-657: Message overstates what this test validates.

Line 655 only tests "lognormal" lowercase, but the expect text says case-insensitive acceptance. Either adjust the message to lowercase-specific behavior or add an uppercase case in this test.

Suggested minimal fix
-            .expect("case-insensitive distribution name should be accepted");
+            .expect("lowercase distribution name should be accepted");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/simulate/common.rs` around lines 655 - 657, The test message
overclaims case-insensitivity for to_family_size_distribution(): either change
the expect() text to only refer to the lowercase input (e.g., "lowercase
distribution name should be accepted") or add an explicit uppercase test by
creating an args_upper (e.g., with "LogNormal" or "LOGNORMAL") and calling
to_family_size_distribution() on it and expecting success; update the
assertion(s) around args_lower and/or add the args_upper assertion so the test
name accurately matches the behavior of to_family_size_distribution().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/commands/simulate/common.rs`:
- Around line 655-657: The test message overclaims case-insensitivity for
to_family_size_distribution(): either change the expect() text to only refer to
the lowercase input (e.g., "lowercase distribution name should be accepted") or
add an explicit uppercase test by creating an args_upper (e.g., with "LogNormal"
or "LOGNORMAL") and calling to_family_size_distribution() on it and expecting
success; update the assertion(s) around args_lower and/or add the args_upper
assertion so the test name accurately matches the behavior of
to_family_size_distribution().

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 16513140-0445-45c4-b033-9e75eaf1ecd0

📥 Commits

Reviewing files that changed from the base of the PR and between ea6e3fc and 9abaf50.

📒 Files selected for processing (27)
  • src/commands/clip.rs
  • src/commands/codec.rs
  • src/commands/common.rs
  • src/commands/compare/bams.rs
  • src/commands/compare/raw_compare.rs
  • src/commands/consensus_runner.rs
  • src/commands/correct.rs
  • src/commands/dedup.rs
  • src/commands/downsample.rs
  • src/commands/duplex.rs
  • src/commands/duplex_metrics.rs
  • src/commands/extract.rs
  • src/commands/fastq.rs
  • src/commands/filter.rs
  • src/commands/group.rs
  • src/commands/merge.rs
  • src/commands/review.rs
  • src/commands/shared_metrics.rs
  • src/commands/simplex.rs
  • src/commands/simplex_metrics.rs
  • src/commands/simulate/common.rs
  • src/commands/simulate/correct_reads.rs
  • src/commands/simulate/fastq_reads.rs
  • src/commands/simulate/grouped_reads.rs
  • src/commands/simulate/mapped_reads.rs
  • src/commands/sort.rs
  • src/commands/zipper.rs

@nh13
nh13 force-pushed the nh/unwrap-audit-commands branch from 9abaf50 to d80b393 Compare April 4, 2026 20:47
@nh13
nh13 temporarily deployed to github-actions April 4, 2026 20:47 — with GitHub Actions Inactive
nh13 added 2 commits April 4, 2026 13:53
Audit and replace all 527 unwrap() calls across 27 files in
src/commands/. Production code unwraps are converted to expect() with
invariant-documenting messages. Test code unwraps are converted to
expect() with context-specific failure messages.

Zero unwrap() calls remain in src/commands/.
The expect message now accurately describes what the function guarantees
rather than restating the obvious precondition check.
@nh13
nh13 force-pushed the nh/unwrap-audit-commands branch from d80b393 to a2ac65f Compare April 4, 2026 20:53
@nh13
nh13 temporarily deployed to github-actions April 4, 2026 20:53 — with GitHub Actions Inactive
@nh13
nh13 merged commit 26df58f into main Apr 4, 2026
8 checks passed
@nh13
nh13 deleted the nh/unwrap-audit-commands branch April 4, 2026 21:00
@nh13 nh13 mentioned this pull request Apr 4, 2026

This branch was previously deployed

1 inactive deployment
github-actions — a2ac65f3 Deployed Apr 4, 2026 by nh13 via coverage #892
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