-
Notifications
You must be signed in to change notification settings - Fork 597
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
NIO output support for HaplotypeCaller #4721
Conversation
Codecov Report
@@ Coverage Diff @@
## master #4721 +/- ##
===========================================
Coverage 80.124% 80.124%
Complexity 17436 17436
===========================================
Files 1082 1082
Lines 63150 63150
Branches 10187 10187
===========================================
Hits 50598 50598
Misses 8568 8568
Partials 3984 3984
|
@droazen this is ready for review! |
ping? It's been a little while, a review would be nice. If @droazen is busy, perhaps @lbergelson can have a look? |
@jean-philippe-martin I'll try to get to this one this week |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a test
@@ -10,6 +10,7 @@ | |||
import htsjdk.samtools.util.Locatable; | |||
import htsjdk.samtools.SamFiles; | |||
|
|||
import java.nio.file.Path; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a test (either here or in HaplotypeCallerIntegrationTest
which asserts that the tool can actually write into the test bucket (similar to the ApplyBQSRIntegrationTest.testApplyBQSRCloud()
test you wrote for a one of your other branches).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I added it to HaplotypeCallerIntegrationTest
, in the "bucket" group.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test looks good 👍. It looks like you have a merge conflict, i'm going to go ahead and fix that quickly and merge
8f1c977
to
bccb477
Compare
Awesome, @jamesemery, thank you very much! |
HaplotypeCaller's bamOutputPath is no longer internally converted to a File so "gs://" paths can now be passed for the optional "bam output" parameter.