/htsjdk

Fix #653 #672

Merged
merged 8 commits into from Aug 10, 2016

Conversation

Reviewers
No reviews
Assignees

@yfarjoun yfarjoun

Labels
Review-party candidate
Projects
None yet
Milestone
No milestone
3 participants
@magicDGS @coveralls @yfarjoun
@magicDGS
Contributor

magicDGS commented Jul 27, 2016 • edited

Simple patch as suggested in #653. TribbleIndexedFeatureReader.isGZIPPath() is not used anymore and it could be removed.

yfarjoun added the Review-party candidate label Aug 9, 2016

yfarjoun was assigned by droazen Aug 9, 2016

@yfarjoun yfarjoun and 1 other commented on an outdated diff Aug 9, 2016

View full outdated diff
.../java/htsjdk/tribble/TribbleIndexedFeatureReader.java
@@ -217,7 +217,7 @@ private void readHeader() throws IOException {
PositionalBufferedStream pbs = null;
try {
is = ParsingUtils.openInputStream(path);
- if (isGZIPPath(path)) {
+ if (hasBlockCompressedExtension(path) || hasBlockCompressedExtension(new URI(path))) {
@yfarjoun

yfarjoun Aug 9, 2016

Contributor

it seems that the first part of if statement is only good for improving performance, but this isn't a performance bottleneck. is it needed for anything else?

@magicDGS

magicDGS Aug 10, 2016 • edited

Contributor

Edited: I was thinking in a different change of code, not in this PR when I write the first comment.
I don't think so, so I will change it. I just reproduced the behaviur of isGZIPPath. I think that it will be even better to modify the AbstractFeatureReader.hasBlockCompressedExtension (final File file) to be something like:

public static boolean hasBlockCompressedExtension (final File file) {
    return hasBlockCompressedExtension(new URI(file));
}

droazen referenced this pull request Aug 9, 2016

Open

Review "Party" #548

@magicDGS
Contributor

magicDGS commented Aug 10, 2016

I addressed your comment and added other stuff:

  • Bgzipped test file (.bgz extension) + test
  • Fixing WFIterator (I forgot to do it in the previous commit)
  • Deprecation of isGZIPPath(final String path)

Because the deprecated method is not used anymore, should I remove it and get rid of the tests too?

Back to you, @yfarjoun!
@magicDGS magicDGS fixing URL encoding

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.3%) to 68.801%
Details
5dd44c8
@coveralls

coveralls commented Aug 10, 2016

Coverage Status

Coverage increased (+0.3%) to 68.801% when pulling 5dd44c8 on magicDGS:dgs_fix_tribblefeature_blockcompression into 87b1e87 on samtools:master.
@magicDGS magicDGS added no-remote test file with spaces

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.3%) to 68.801%
Details
a21f906
@coveralls

coveralls commented Aug 10, 2016

Coverage Status

Coverage increased (+0.3%) to 68.801% when pulling a21f906 on magicDGS:dgs_fix_tribblefeature_blockcompression into 87b1e87 on samtools:master.

@yfarjoun yfarjoun and 1 other commented on an outdated diff Aug 10, 2016

View full outdated diff
.../java/htsjdk/tribble/TribbleIndexedFeatureReader.java
@@ -274,6 +275,7 @@ private void readHeader() throws IOException {
}
//Visible for testing
+ @Deprecated
@yfarjoun

yfarjoun Aug 10, 2016

Contributor

if deprecating, please comment what should be used instead.

@magicDGS

magicDGS Aug 10, 2016

Contributor

Done, but because it is package specific I wasn't sure if it will be better to remove...
@magicDGS magicDGS added comment for deprecated method

All checks have passed

2 successful checks
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
coverage/coveralls — Coverage increased (+0.3%) to 68.801%
Details
a4ea9aa
@coveralls

coveralls commented Aug 10, 2016

Coverage Status

Coverage increased (+0.3%) to 68.801% when pulling a4ea9aa on magicDGS:dgs_fix_tribblefeature_blockcompression into 87b1e87 on samtools:master.
@magicDGS
Contributor

magicDGS commented Aug 10, 2016

@yfarjoun, I added the comment for the deprecated method.

@yfarjoun yfarjoun merged commit c8202f2 into samtools:master Aug 10, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.3%) to 68.801%
Details
@yfarjoun
Contributor

yfarjoun commented Aug 10, 2016

💯

@jamesemery jamesemery added a commit to jamesemery/htsjdk that referenced this pull request Sep 1, 2016

@magicDGS @jamesemery magicDGS + jamesemery Fix #653 (#672) 
* added checking for AbstractFeatureReader.BLOCK_COMPRESSED_EXTENSIONS in TribbleIndexedFeatureReader

* fixing WFIterator checking of compressed file

* isGZIPPath deprecation

* added test for new functionality

* fixing URL encoding

* added no-remote test file with spaces
 ffd8427
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment