Vcf support #308
Vcf support #308
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #308 +/- ##
==========================================
+ Coverage 59.56% 59.88% +0.31%
==========================================
Files 29 29
Lines 4078 4170 +92
Branches 484 481 -3
==========================================
+ Hits 2429 2497 +68
- Misses 1165 1192 +27
+ Partials 484 481 -3 ☔ View full report in Codecov by Sentry. |
|
OK, this was begun a while ago, and only tests were missing. I've added one now. Note that as with GZIP, this PR only implements reading of VCF files, not yet of writing. But I think this would already have a number of use cases that are important, so I would vote for getting this into a release even before we support writing. There are also a number of decisions I have taken that need to stand the test of time. For example, since VCF does not contain information on group names or Sex, I had to tweak the Janno-Ind consistency check function. Also, VCF reading is somewhat difficult to implement in its full glory of the VCF spec. The Spec is very flexible and complex, and of course my I would like to advertise this feature as "experimental" for now and first have it tested for a while before we move on and review some of the decisions I've made. I hope it won't be too much work to review this PR. Happy to give you a run-through in person, @nevrome, if that helps. |
There was a problem hiding this comment.
The code looks solid. But there are a lot changes in the CLI subcommands that require good testing. I hope the golden tests cover everything important...
Maybe it's worth to write some more golden tests explicitly for the .vcf input.
I also think the POSEIDON_* test data hack is really reaching its limits here. We should have proper test packages for these increasingly different setups!
| parseFileWithEndings :: String -> [String] -> OP.Parser FilePath | ||
| parseFileWithEndings long endings = OP.option (OP.maybeReader fileEndingReader) ( | ||
| OP.long long <> | ||
| OP.help ("Accepted file endings are " ++ intercalate ", " endings) <> |
There was a problem hiding this comment.
This "universal" parser function is a good idea. Scales really well. Maybe the "Accepted file endings are ..." help text is a bit too minimalist.
There was a problem hiding this comment.
True. I've now added a bit more specific help texts. Take a look.
|
Thanks for the review. Golden tests are a good idea, and I will also refactor those test packages. |
|
OK, I've restructured the test packages and added Golden tests. I've also updated the version number, Changelog and Releasechangelog. Have a look. |
|
Perfect! I also ran some little experiments on the command line and could find no issues. So I'll merge this now and trigger the release. |
Draft PR for VCF reading support