refactor(commands): deduplicate simulate utilities and clip header logic

[nh13]

Summary

• Move generate_random_sequence, pad_sequence, compute_position, and MoleculeInfo from 5 simulate subcommands into shared common.rs
• Replace 3 local reverse_complement implementations with fgumi_dna::dna::reverse_complement library function
• Simplify reverse_complement_into in fastq_reads.rs to use complement_base from fgumi-dna
• Replace 37-line inline header sort order update in clip.rs with call to existing update_header_sort_order method

Test plan

• All 1838 tests pass (cargo ci-test)
• Formatting clean (cargo ci-fmt)
• Linting clean (cargo ci-lint)
• Net removal of ~153 lines

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.72%. Comparing base (3a460ea) to head (d9d5596).
⚠️ Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #155      +/-   ##
==========================================
+ Coverage   83.61%   83.72%   +0.10%     
==========================================
  Files         126      126              
  Lines       51510    51286     -224     
==========================================
- Hits        43069    42937     -132     
+ Misses       8441     8349      -92     

☔ 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]

📝 Walkthrough

Walkthrough

Consolidates duplicated simulation helpers into src/commands/simulate/common.rs: generate_random_sequence, pad_sequence, compute_position, and MoleculeInfo (module-private). Updates consensus_reads.rs, correct_reads.rs, fastq_reads.rs, grouped_reads.rs, and mapped_reads.rs to import and use these shared utilities and to replace local reverse_complement implementations with imports from fgumi_lib::dna. Adds tests for the new utilities and updates reverse-complement test expectations. Simplifies clip.rs by replacing inline header mutation with a single update_header_sort_order helper.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title accurately describes the main refactoring work: deduplicating simulate utilities and simplifying clip header logic across multiple files.
Description check	✅ Passed	Description directly relates to the changeset, detailing the specific utilities moved, library functions substituted, and line reductions achieved.
Docstring Coverage	✅ Passed	Docstring coverage is 92.59% 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 (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch refactor/nh/simplify-fgumi-commands-postproc

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/simulate/common.rs`:
- Around line 231-233: compute_position currently can panic if num_positions ==
0 (modulo by zero) and can underflow/produce incorrect values when ref_length <
1000; update the function to validate inputs: return a Result or clamp/fallback
values if invalid. Specifically, in compute_position(mol_id, num_positions,
ref_length) check num_positions != 0 before computing position_idx (or return
Err) and ensure ref_length >= 1000 (or treat ref_length < 1000 as ref_length =
1000 or return Err) so the expression ((position_idx as f64 / num_positions as
f64) * (ref_length - 1000) as f64) as usize + 100 cannot underflow; update
callers to handle the Result if you choose to return one.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3a460ea and 107e439.

📒 Files selected for processing (7)

• src/commands/clip.rs
• src/commands/simulate/common.rs
• src/commands/simulate/consensus_reads.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

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (2)

src/commands/clip.rs (1)

95-96: Validate --sort-order values at parse time.

sort_order is free-form and gets written into @HD:SO directly. Constraining accepted values avoids emitting non-standard headers.

Proposed diff

-    #[arg(short = 'S', long = "sort-order")]
-    pub sort_order: Option<String>,
+    #[arg(
+        short = 'S',
+        long = "sort-order",
+        value_parser = ["unknown", "unsorted", "queryname", "coordinate"]
+    )]
+    pub sort_order: Option<String>,

Also applies to: 585-600

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/clip.rs` around lines 95 - 96, The sort_order field is free-form
and should be validated at parse time; replace the Option<String> sort_order in
the Clip command with a typed enum (e.g., enum SortOrder { Unknown, Unsorted,
Queryname, Coordinate }) and derive/implement clap parsing (ValueEnum or FromStr
+ #[arg(value_parser)]). Implement FromStr or use clap::ValueEnum for SortOrder
to only accept the SAM `@HD`:SO standard values
("unknown","unsorted","queryname","coordinate"), update the Clip struct field to
Option<SortOrder>, and adjust any code that writes `@HD`:SO to map SortOrder ->
string; apply the same change to the other occurrence(s) of sort_order in this
file (the block around the later instance).

src/commands/simulate/common.rs (1)

652-674: Use rstest for this compute-position case matrix.

These cases are a parameterized table and fit rstest directly.

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/simulate/common.rs` around lines 652 - 674, Replace the three
individual unit tests for compute_position with a single parameterized rstest
table: create an rstest function (e.g., #[rstest] fn
test_compute_position(index: usize, num_positions: usize, ref_length: usize,
expected: ExpectedType) { ... }) that calls compute_position(index,
num_positions, ref_length) and asserts the expected result or range; include
rows for the cases from the diff (index=5,num_positions=0,ref_length=250_000_000
-> expect 100; index=0,num_positions=10,ref_length=500 -> expect 100;
index=0,num_positions=10,ref_length=10_000 -> expect 100;
index=9,num_positions=10,ref_length=10_000 -> expect a value >100 and <10_000).
Ensure you import rstest and adjust assertions to support both exact and range
checks inside the single parametrized test, referencing compute_position by
name.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/simulate/common.rs`:
- Around line 231-240: The fallback logic in compute_position returns 100 when
num_positions is zero or usable_span is zero, which can produce out-of-range
start positions for small references; modify compute_position to compute the
candidate start (currently "((position_idx as f64 / num_positions as f64) *
usable_span as f64) as usize + 100") but then clamp the resulting start to the
valid range [0, ref_length.saturating_sub(1)] (or at least ensure it is >= 0 and
<= ref_length - 1) and when returning the fallback for zero span use a clamped
value like min(100, ref_length.saturating_sub(1)) so all returns from
compute_position are within the reference bounds.

---

Nitpick comments:
In `@src/commands/clip.rs`:
- Around line 95-96: The sort_order field is free-form and should be validated
at parse time; replace the Option<String> sort_order in the Clip command with a
typed enum (e.g., enum SortOrder { Unknown, Unsorted, Queryname, Coordinate })
and derive/implement clap parsing (ValueEnum or FromStr + #[arg(value_parser)]).
Implement FromStr or use clap::ValueEnum for SortOrder to only accept the SAM
`@HD`:SO standard values ("unknown","unsorted","queryname","coordinate"), update
the Clip struct field to Option<SortOrder>, and adjust any code that writes
`@HD`:SO to map SortOrder -> string; apply the same change to the other
occurrence(s) of sort_order in this file (the block around the later instance).

In `@src/commands/simulate/common.rs`:
- Around line 652-674: Replace the three individual unit tests for
compute_position with a single parameterized rstest table: create an rstest
function (e.g., #[rstest] fn test_compute_position(index: usize, num_positions:
usize, ref_length: usize, expected: ExpectedType) { ... }) that calls
compute_position(index, num_positions, ref_length) and asserts the expected
result or range; include rows for the cases from the diff
(index=5,num_positions=0,ref_length=250_000_000 -> expect 100;
index=0,num_positions=10,ref_length=500 -> expect 100;
index=0,num_positions=10,ref_length=10_000 -> expect 100;
index=9,num_positions=10,ref_length=10_000 -> expect a value >100 and <10_000).
Ensure you import rstest and adjust assertions to support both exact and range
checks inside the single parametrized test, referencing compute_position by
name.

---

ℹ️ Review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 107e439 and d9d5596.

📒 Files selected for processing (7)

• src/commands/clip.rs
• src/commands/simulate/common.rs
• src/commands/simulate/consensus_reads.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

🚧 Files skipped from review as they are similar to previous changes (4)

• src/commands/simulate/correct_reads.rs
• src/commands/simulate/fastq_reads.rs
• src/commands/simulate/mapped_reads.rs
• src/commands/simulate/consensus_reads.rs