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

Removed redundant readability test in unrollPaths method #1355

Merged
merged 2 commits into from
May 20, 2019

Conversation

yfarjoun
Copy link
Contributor

Cloud paths are slow to provide readability. This PR removes a readability check that comes right before opening a file. A failure to read the file will result in an error in both cases.

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

@yfarjoun One comment. Feel free to merge after that.

@@ -1107,7 +1137,7 @@ public static String slurp(final InputStream is, final Charset charSet) {
throw new IllegalArgumentException("had trouble reading from " + p.toUri().toString());
Copy link
Member

Choose a reason for hiding this comment

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

Could you add the exception e as the cause to this newly thrown exception.

@codecov-io
Copy link

codecov-io commented May 20, 2019

Codecov Report

Merging #1355 into master will increase coverage by 0.716%.
The diff coverage is 25%.

@@               Coverage Diff               @@
##              master     #1355       +/-   ##
===============================================
+ Coverage     67.848%   68.564%   +0.716%     
- Complexity      8283      8805      +522     
===============================================
  Files            563       564        +1     
  Lines          33706     35024     +1318     
  Branches        5657      6012      +355     
===============================================
+ Hits           22869     24014     +1145     
- Misses          8659      8783      +124     
- Partials        2178      2227       +49
Impacted Files Coverage Δ Complexity Δ
src/main/java/htsjdk/samtools/util/IOUtil.java 58.111% <25%> (-0.101%) 118 <0> (ø)
...s/cram/encoding/external/ExternalLongEncoding.java 61.538% <0%> (-5.128%) 4% <0%> (+1%)
src/main/java/htsjdk/variant/bcf2/BCFVersion.java 93.103% <0%> (-1.633%) 12% <0%> (+5%)
src/main/java/htsjdk/samtools/SAMFileHeader.java 66.942% <0%> (-0.5%) 83% <0%> (+38%)
...dk/samtools/cram/structure/SubstitutionMatrix.java 91.026% <0%> (-0.279%) 30% <0%> (+15%)
...s/cram/encoding/external/ExternalByteEncoding.java 100% <0%> (ø) 10% <0%> (+4%) ⬆️
...ram/encoding/external/ExternalIntegerEncoding.java 100% <0%> (ø) 10% <0%> (+4%) ⬆️
...tools/cram/encoding/external/ExternalEncoding.java 100% <0%> (ø) 2% <0%> (?)
...c/main/java/htsjdk/samtools/util/IntervalList.java 73.913% <0%> (+0.016%) 103% <0%> (+34%) ⬆️
...n/java/htsjdk/samtools/cram/structure/SliceIO.java 84.524% <0%> (+1.19%) 15% <0%> (+1%) ⬆️
... and 17 more

@yfarjoun yfarjoun merged commit 64e98d6 into master May 20, 2019
@yfarjoun yfarjoun deleted the yf_remove_redundant_isreadable branch May 20, 2019 19:42
@yfarjoun yfarjoun restored the yf_remove_redundant_isreadable branch August 11, 2020 16:14
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.

3 participants