Skip to content
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

resolving some nitpicks from pr #1245 #1246

Merged
merged 2 commits into from
Dec 21, 2018
Merged

Conversation

lbergelson
Copy link
Member

resolving some nitpicks left over from pr #1245

@codecov-io
Copy link

codecov-io commented Dec 19, 2018

Codecov Report

Merging #1246 into master will increase coverage by 0.004%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master     #1246       +/-   ##
===============================================
+ Coverage     69.407%   69.412%   +0.004%     
  Complexity      8157      8157               
===============================================
  Files            548       548               
  Lines          32691     32689        -2     
  Branches        5521      5521               
===============================================
  Hits           22690     22690               
+ Misses          7783      7781        -2     
  Partials        2218      2218
Impacted Files Coverage Δ Complexity Δ
src/main/java/htsjdk/samtools/SamStreams.java 72.308% <ø> (ø) 19 <0> (ø) ⬇️
src/main/java/htsjdk/samtools/util/IOUtil.java 57.143% <ø> (ø) 115 <0> (ø) ⬇️
...in/java/htsjdk/variant/vcf/VCFIteratorBuilder.java 94.286% <100%> (-0.309%) 9 <0> (ø)
...ain/java/htsjdk/variant/utils/VCFHeaderReader.java 100% <100%> (ø) 6 <0> (ø) ⬇️
...va/htsjdk/samtools/sra/SRAIndexedSequenceFile.java 62.162% <0%> (-2.703%) 6% <0%> (-1%)
...samtools/util/AsyncBlockCompressedInputStream.java 76% <0%> (+4%) 13% <0%> (+1%) ⬆️

@romanzenka
Copy link
Contributor

romanzenka commented Dec 20, 2018

Looks like there is a tiiiny coverage drop thanks to an exception being thrown when stream does not have "markSupported". And apparently the MD5 hashing somehow shook out to work so that nobody ever hits MD5 hashes shorter than 32 so no need to pad them (?)... I can try helping with that if you want, but it looks very minor.

@lbergelson
Copy link
Member Author

@romanzenka Thank you for the offer, I wouldn't worry about that, our coverage detection always fails do to minor changes. I need to fix it...

Copy link
Contributor

@yfarjoun yfarjoun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nits on your nits.

@@ -99,6 +99,8 @@
public static final String COMPRESSED_VCF_INDEX_EXTENSION = ".tbi";



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why extra nl?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

accident...

@@ -50,7 +51,7 @@ public static VCFHeader readHeaderFrom(final SeekableStream in) throws IOExcepti
private static InputStream bufferAndDecompressIfNecessary(final InputStream in) throws IOException {
BufferedInputStream bis = new BufferedInputStream(in);
// despite the name, SamStreams.isGzippedSAMFile looks for any gzipped stream (including block compressed)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update comment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@lbergelson lbergelson merged commit a4b7da8 into master Dec 21, 2018
@lbergelson lbergelson deleted the lb_resolve_nitpicks branch December 21, 2018 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants