new Interface VCFIterator extends CloseableIterator<VariantContext> (continued) #1245
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- better @see syntax - extra open(String, wrapper) method in VCFIteratorBuilder - IOTestUtil tests isGZIPInputStream function using a @dataProvider
Codecov Report
@@ Coverage Diff @@
## master #1245 +/- ##
===============================================
+ Coverage 69.359% 69.404% +0.045%
- Complexity 8143 8156 +13
===============================================
Files 547 548 +1
Lines 32649 32691 +42
Branches 5517 5521 +4
===============================================
+ Hits 22645 22689 +44
+ Misses 7784 7783 -1
+ Partials 2220 2219 -1
|
lbergelson
approved these changes
Dec 19, 2018
There was a problem hiding this comment.
@romanzenka Thank you for taking this on and doing the rebase / resolving the last round of comments. @lindenb Thank you for doing this work initially.
I wrote a bunch of nitpicky comments, but i'm going to resolve them myself in a future pr rather than delay this longer.
| @@ -66,26 +66,11 @@ public static boolean isBAMFile(final InputStream stream) | |||
| /** | |||
| * Checks whether the file is a gzipped sam file. Returns true if it | |||
| * is and false otherwise. | |||
| * @see @link IOUtil#isGZIPInputStream(InputStream) | |||
There was a problem hiding this comment.
Suggested change
| * @see @link IOUtil#isGZIPInputStream(InputStream) | |
| * @deprecated use {@link IOUtil#isGZIPInputStream(InputStream)} instead |
| @@ -1157,6 +1155,38 @@ public static Path toPath(File fileOrNull) { | |||
| return files.stream().map(File::toPath).collect(Collectors.toList()); | |||
| } | |||
|
|
|||
| /** number of bytes that will be read for the GZIP-header in the function {@link #isGZIPInputStream(InputStream)} */ | |||
| public static final int GZIP_HEADER_READ_LENGTH = 8000; | |||
There was a problem hiding this comment.
Could you stick this at the top of the file with the other constants?
| * @param stream the input stream. | ||
| * @return true if `stream` starts with a gzip signature | ||
| * @throws IllegalArgumentException if `stream` cannot mark or reset the stream | ||
| * @see SamStreams#isGzippedSAMFile(InputStream) |
There was a problem hiding this comment.
Suggested change
| * @see SamStreams#isGzippedSAMFile(InputStream) |
We can drop this pointer to the old method.
| * @see SamStreams#isGzippedSAMFile(InputStream) | ||
| */ | ||
| public static boolean isGZIPInputStream(final InputStream stream) { | ||
| /* this function was previously implemented in SamStreams.isGzippedSAMFile */ |
There was a problem hiding this comment.
You don't need this comment.
Suggested change
| /* this function was previously implemented in SamStreams.isGzippedSAMFile */ |
| // wrap the input stream into a BufferedInputStream to reset/read a BCFHeader or a GZIP | ||
| // buffer must be large enough to contain the BCF header and/or GZIP signature | ||
| BufferedInputStream bufferedinput = new BufferedInputStream(in, Math.max( | ||
| BCF2Codec.SIZEOF_BCF_HEADER, |
There was a problem hiding this comment.
I would just put these one one line, but that's just a very minor nitpick.
| if(IOUtil.isGZIPInputStream(bufferedinput)) { | ||
| // this is a gzipped input stream, wrap it into GZIPInputStream | ||
| // and re-wrap it into BufferedInputStream so we can test for the BCF header | ||
| bufferedinput = new BufferedInputStream( |
| throw new IllegalStateException("Could not reset stream."); | ||
| } | ||
| } | ||
| return IOUtil.isGZIPInputStream(stream); |
There was a problem hiding this comment.
There's a new use of this deprecated method in VCFHeaderReader. It should be migrated.
lbergelson
added a commit
that referenced
this pull request
Dec 19, 2018
|
🎉 |
lbergelson
added a commit
that referenced
this pull request
Dec 19, 2018
lbergelson
added a commit
that referenced
this pull request
Dec 19, 2018
|
I was going to resolve the "nitpicks", but it looks like you've done it alraedy. Thank you! |
lbergelson
added a commit
that referenced
this pull request
Dec 21, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This is a PR based on #837 - the only difference is addressing the reviewer comments + a little bit of extra testing.
My purpose is to continue "carrying the torch" of the original PR by @lindenb until successful completion, as I could REALLY use the VCFIterator interface.
Checklist