Expose a couple of protected methods and replace hard coded strings w…

[tfenne]

…ith an enum. Take 2!

Description

I have some programs that use FastqReader that are failing when they encounter fastqs with empty seq or quality lines. Since there is no complete formal spec for fastq, it's hard to argue that this is invalid (though is silly IMHO) - and this situation seems to be created by tools that quality trim paired fastq files when one read is all low quality (e.g. R2) and the other (e.g. R1) is fine.

I've made the simplest change possible here, which is to expose the method that asserts the lines are non-empty, so that I can subclass and allow empty Seq and Qual lines, rather than trying to bake that into the reader which would require potentially a lot more changes.

Functionality is unchanged; a couple of methods go from private to protected, and I've replace four hard-coded strings for the line types with an enum so that subclasses don't have to depend on string matching.

Checklist

• Code compiles correctly
• New tests covering changes and new functionality
• All tests passing
• Extended the README / documentation, if necessary
• Is not backward compatible (breaks binary or source compatibility)

[codecov-io]

Codecov Report

Merging #854 into master will increase coverage by 0.081%.
The diff coverage is 85%.

@@               Coverage Diff               @@
##              master      #854       +/-   ##
===============================================
+ Coverage     64.835%   64.916%   +0.081%     
- Complexity      7188      7201       +13     
===============================================
  Files            527       527               
  Lines          31779     31784        +5     
  Branches        5425      5425               
===============================================
+ Hits           20604     20633       +29     
+ Misses          9035      9014       -21     
+ Partials        2140      2137        -3

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/htsjdk/samtools/fastq/FastqReader.java	81.081% <85%> (+31.806%)	24 <3> (+11)	⬆️
...samtools/util/AsyncBlockCompressedInputStream.java	72% <0%> (-4%)	12% <0%> (-1%)
src/main/java/htsjdk/samtools/util/StringUtil.java	66.154% <0%> (+0.513%)	65% <0%> (+1%)	⬆️
src/main/java/htsjdk/samtools/util/IOUtil.java	30.719% <0%> (+0.654%)	59% <0%> (ø)	⬇️
...dk/samtools/seekablestream/SeekableHTTPStream.java	56.061% <0%> (+1.515%)	10% <0%> (+1%)	⬆️
src/main/java/htsjdk/samtools/QueryInterval.java	72.973% <0%> (+5.405%)	15% <0%> (+1%)	⬆️

[tfenne]

Fixed it @yfarjoun - seems like I'd managed to create the new test class in the wrong source root (java vs. scala) and somehow it got compiled and run locally, but not on travis.

[yfarjoun]

So the lesson here is that scala-tests should be placed under the scala root and java-tests under java, and also that not doing so seems to skip the incorrectly placed tests, correct?

Should we have a target that explicitly tests that so that we do not get cases like this in the future?

[yfarjoun]

thanks.

[yfarjoun]

here too, just to be sure. also

IOUtil.slurpLines(path.toFile) should have size 4

[yfarjoun]

thanks for the corrected PR.

small request for the empty fastq and then

👍

[tfenne]

I'm not sure if there's a good way to test for scala files under the Java root. This was caused (in my case) by IntelliJ trying to be too smart. If you have multiple source roots (java and scala) and one of them already contains a package and the other doesn't it does funny things when you try to create a new class (either auto-using the one root that has the package, or offering a drop-down that contains only that root). We could probably try and add something to the build that catches .scala files under src/test/java.

[yfarjoun]

I think we could add a script that scans for .scala files anywhere except the test/scala subdir and .java files there. if it finds any it fails. Then we can add this target to the test targets, so that it will fail tests.

…

On Sun, Apr 16, 2017 at 10:05 AM, Tim Fennell ***@***.***> wrote: I'm not sure if there's a good way to test for scala files under the Java root. This was caused (in my case) by IntelliJ trying to be too smart. If you have multiple source roots (java and scala) and one of them already contains a package and the other doesn't it does funny things when you try to create a new class (either auto-using the one root that has the package, or offering a drop-down that contains only that root). We could probably try and add something to the build that catches .scala files under src/test/java. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#854 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACnk0n5q4Qhhjma1IX_J8PHYa3kcFNv4ks5rwiA8gaJpZM4M-m8K> .

[tfenne]

I agree, I’m just not really sure how to do that in Gradle.