Validate VCF sample options and other CLI args upfront#2
Conversation
Previously, an invalid --sample name or a multi-sample VCF without --sample would panic or produce a cryptic error deep in the per-contig simulation loop. Now these checks run during argument validation so the user gets a clear, actionable error message immediately. - Add validate_vcf_sample() to open the VCF header and verify sample configuration before simulation begins - Improve error messages to list available sample names - Reject --sample without --vcf - Reject output prefix when parent directory doesn't exist - Add write_vcf_header_only() test helper for multi-sample VCFs - Add 6 integration tests covering all validation paths
📝 WalkthroughWalkthroughThe changes add preflight validation to the Changes
Sequence DiagramsequenceDiagram
participant User as User/CLI
participant Simulate as Simulate::validate
participant VCF as vcf::validate_vcf_sample
participant Output as Output Dir Check
User->>Simulate: simulate --sample X --vcf file.vcf --output prefix
Simulate->>Simulate: Check --sample without --vcf
alt --sample without --vcf
Simulate-->>User: Error: bail!
end
Simulate->>VCF: validate_vcf_sample(path, sample_name)
VCF->>VCF: Open VCF, read header
VCF->>VCF: resolve_sample_index(sample_name)
alt Sample not found
VCF-->>Simulate: Error with available samples
Simulate-->>User: Error: bail!
end
Simulate->>Output: Check parent directory exists
alt Directory missing
Output-->>Simulate: Error
Simulate-->>User: Error: bail!
end
alt All validations pass
Simulate-->>User: Proceed to run_simulation
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_simulate.rs (1)
2976-3111: Add///doc comments to the six new validation tests.The new test cases are non-trivial and should follow the same doc-comment style used by the surrounding tests for consistency and discoverability.
As per coding guidelines, "Add doc comments on all public and non-trivial private items".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_simulate.rs` around lines 2976 - 3111, Add triple-slash doc comments (///) above each of the six new tests explaining what the test validates: place short one-line descriptions above test_sample_without_vcf_fails, test_wrong_sample_name_fails, test_multi_sample_vcf_without_sample_flag_fails, test_single_sample_vcf_without_sample_flag_works, test_multi_sample_vcf_with_correct_sample_works, and test_output_directory_does_not_exist_fails that mirror surrounding tests' style (e.g., "Checks that ... fails/works when ..."), keeping them concise and focused on the behavior asserted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_simulate.rs`:
- Around line 2976-3111: Add triple-slash doc comments (///) above each of the
six new tests explaining what the test validates: place short one-line
descriptions above test_sample_without_vcf_fails, test_wrong_sample_name_fails,
test_multi_sample_vcf_without_sample_flag_fails,
test_single_sample_vcf_without_sample_flag_works,
test_multi_sample_vcf_with_correct_sample_works, and
test_output_directory_does_not_exist_fails that mirror surrounding tests' style
(e.g., "Checks that ... fails/works when ..."), keeping them concise and focused
on the behavior asserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 954a622b-5651-49c3-aaef-f4b3f81a3407
📒 Files selected for processing (4)
src/commands/simulate.rssrc/vcf/mod.rstests/helpers/mod.rstests/test_simulate.rs
Summary
--samplename exists in the VCF header upfront, before the simulation loop, with an error message that lists available samples--sampleto be specified (and that single-sample VCFs work without it)--samplewithout--vcfwrite_vcf_header_only()test helper for creating multi-sample VCFsTest plan
cargo ci-fmtpassescargo ci-lintpassescargo ci-testpasses (177/177 tests, including 6 new validation tests)Summary by CodeRabbit
--sampleand--vcfoption combinations with clearer error messages