Skip to content

refactor(clip): use ClippingMode enum instead of String for --clipping-mode - #218

Merged
nh13 merged 1 commit into
mainfrom
nh/refactor-clipping-mode-enum
Apr 2, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/refactor-clipping-mode-enum

Conversation

@nh13

@nh13 nh13 commented Apr 2, 2026

Copy link
Copy Markdown
Member

Summary

  • Derive clap::ValueEnum on the existing ClippingMode enum so that clap validates the --clipping-mode argument at parse time instead of at runtime
  • Remove two manual match self.clipping_mode.as_str() blocks in clip.rs (one in execute(), one in the pipeline closure)
  • Change clipping_mode field type from String to ClippingMode in the Clip CLI struct
  • Add Display impl on ClippingMode for the info log line
  • Remove the now-unnecessary test_clip_execute_invalid_clipping_mode test (invalid values are rejected by clap at parse time)

Test plan

  • cargo ci-fmt passes
  • cargo ci-lint passes
  • cargo nextest run --no-fail-fast — 1734 tests pass, 7 skipped
  • Verify fgumi clip --help shows the valid values for --clipping-mode
  • Verify fgumi clip --clipping-mode invalid ... is rejected by clap with a helpful error

…g-mode

Derive clap::ValueEnum on ClippingMode so clap handles parsing and
validation at argument-parse time. This removes two manual match blocks
in clip.rs, eliminates the possibility of runtime "invalid clipping
mode" errors, and gives users automatic shell completions and help text
for the valid variants.
@nh13
nh13 temporarily deployed to github-actions April 2, 2026 21:30 — with GitHub Actions Inactive
@nh13
nh13 marked this pull request as ready for review April 2, 2026 21:32
@codecov

codecov Bot commented Apr 2, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 98.11321% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.06%. Comparing base (f13e78b) to head (850b2ad).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/commands/clip.rs 98.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #218      +/-   ##
==========================================
- Coverage   88.07%   88.06%   -0.02%     
==========================================
  Files         113      113              
  Lines       52863    52816      -47     
==========================================
- Hits        46561    46511      -50     
- Misses       6302     6305       +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.

@coderabbitai

coderabbitai Bot commented Apr 2, 2026

Copy link
Copy Markdown
📝 Walkthrough

Walkthrough

The changes add clap v4 as a dependency and refactor clipping mode handling from string-based to enum-based. The ClippingMode enum now derives clap::ValueEnum, implements Display, and includes a custom CLI value name mapping for SoftWithMask. The Clip command struct's clipping_mode field transitions from String to ClippingMode, replacing string validation and parsing logic with direct enum usage. Tests are updated to use enum variants instead of string literals, and invalid mode tests are removed.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly and concisely describes the main refactor: moving from String to ClippingMode enum for the --clipping-mode argument.
Description check ✅ Passed Description is directly relevant, detailing the specific changes made, rationale, and test plan for the clipping mode enum refactor.
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 docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch nh/refactor-clipping-mode-enum

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.

@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/clip.rs (1)

1081-1108: Parameterize the enum-mode test with rstest.

This test currently checks one case and is a good fit for rstest cases over all variants.

♻️ Suggested refactor
-    #[test]
-    fn test_clipping_mode_enum_values() {
-        // Test that clipping_mode enum variants are set properly
-        let soft = Clip {
+    #[rstest]
+    #[case(ClippingMode::Soft)]
+    #[case(ClippingMode::SoftWithMask)]
+    #[case(ClippingMode::Hard)]
+    fn test_clipping_mode_enum_values(#[case] mode: ClippingMode) {
+        let clip = Clip {
             io: BamIoOptions {
                 input: PathBuf::from("input.bam"),
                 output: PathBuf::from("output.bam"),
             },
             reference: PathBuf::from("reference.fa"),
-            clipping_mode: ClippingMode::Soft,
+            clipping_mode: mode,
             clip_overlapping_reads: true,
             clip_extending_past_mate: false,
@@
             scheduler_opts: SchedulerOptions::default(),
             queue_memory: QueueMemoryOptions::default(),
         };
 
-        assert_eq!(soft.clipping_mode, ClippingMode::Soft);
+        assert_eq!(clip.clipping_mode, mode);
     }

As per coding guidelines "**/*.rs: Use rstest for parameterized tests".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/clip.rs` around lines 1081 - 1108, Replace the single-case
test_clipping_mode_enum_values with an rstest-parameterized test: add the
#[rstest] attribute (and import rstest::rstest) and change the function
signature to accept a clipping_mode: ClippingMode parameter with a #[case(...)]
for each ClippingMode variant; inside the test construct the Clip using that
clipping_mode (same construction as before) and assert_eq!(clipping_mode,
clip.clipping_mode). Keep the test name (test_clipping_mode_enum_values) and
reference the Clip struct and ClippingMode enum so the IDE can locate the code.
🤖 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/clip.rs`:
- Around line 1081-1108: Replace the single-case test_clipping_mode_enum_values
with an rstest-parameterized test: add the #[rstest] attribute (and import
rstest::rstest) and change the function signature to accept a clipping_mode:
ClippingMode parameter with a #[case(...)] for each ClippingMode variant; inside
the test construct the Clip using that clipping_mode (same construction as
before) and assert_eq!(clipping_mode, clip.clipping_mode). Keep the test name
(test_clipping_mode_enum_values) and reference the Clip struct and ClippingMode
enum so the IDE can locate the code.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: eecbaf1d-a2b4-4854-932b-02fd7e34f098

📥 Commits

Reviewing files that changed from the base of the PR and between f13e78b and 850b2ad.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (3)
  • crates/fgumi-sam/Cargo.toml
  • crates/fgumi-sam/src/clipper.rs
  • src/commands/clip.rs

@nh13
nh13 merged commit 24182af into main Apr 2, 2026
7 checks passed
@nh13
nh13 deleted the nh/refactor-clipping-mode-enum branch April 2, 2026 21:55
@nh13 nh13 mentioned this pull request Apr 1, 2026

This branch was previously deployed

1 inactive deployment
github-actions — 850b2ad8 Deployed Apr 2, 2026 by nh13 via coverage #825
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