/htsjdk

Index.writeBasedOnFeaturePath should throw instead of silently Failing #841

Merged
merged 5 commits into from May 27, 2017

Conversation

Reviewers

@droazen droazen

@lbergelson lbergelson

Assignees

@yfarjoun yfarjoun

Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@magicDGS @codecov-io @droazen @lbergelson @yfarjoun
@magicDGS
Contributor

magicDGS commented Apr 4, 2017 • edited

Description

Solves #821

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)
@magicDGS
Contributor

magicDGS commented Apr 4, 2017

Here you have a patch that should be tested against picard/gatk, @lbergelson. Silently failing is not nice...
@codecov-io

codecov-io commented Apr 4, 2017 • edited

Codecov Report

Merging #841 into master will increase coverage by 0.014%.
The diff coverage is 100%.

@@               Coverage Diff               @@
##              master      #841       +/-   ##
===============================================
+ Coverage     64.939%   64.953%   +0.014%     
- Complexity      7210      7211        +1     
===============================================
  Files            527       527               
  Lines          31802     31800        -2     
  Branches        5426      5426               
===============================================
+ Hits           20652     20655        +3     
+ Misses          9017      9013        -4     
+ Partials        2133      2132        -1
Impacted Files Coverage Δ Complexity Δ
src/main/java/htsjdk/tribble/index/Index.java 100% <ø> (ø) 2 <0> (ø) ⬇️
.../main/java/htsjdk/tribble/index/AbstractIndex.java 52.91% <100%> (+1.331%) 31 <0> (+1) ⬆️
...in/java/htsjdk/tribble/index/tabix/TabixIndex.java 78.788% <100%> (+1.679%) 39 <0> (+1) ⬆️
...dk/samtools/seekablestream/SeekableHTTPStream.java 54.545% <0%> (-1.515%) 9% <0%> (-1%)
src/main/java/htsjdk/tribble/index/tabix/TabixIndex.java
@@ -230,8 +228,7 @@ public void write(final Path tabixPath) throws IOException {
@Override
public void writeBasedOnFeaturePath(final Path featurePath) throws IOException {
@droazen

droazen Apr 18, 2017 • edited

Contributor

Can you update the javadoc for this method (and overloads) in the Index interface to mention that this method now throws an IOException in the case where featurePath is not a regular file?

@lbergelson

lbergelson Apr 18, 2017

Contributor

also update the javadoc for the associated file methods

@magicDGS

magicDGS Apr 23, 2017

Contributor

Done.

src/main/java/htsjdk/tribble/index/tabix/TabixIndex.java
@@ -230,8 +228,7 @@ public void write(final Path tabixPath) throws IOException {
@Override
public void writeBasedOnFeaturePath(final Path featurePath) throws IOException {
if (!Files.isRegularFile(featurePath)) {
- LOGGER.warn("Index not written into ", featurePath);
- return;
+ throw new IOException("Cannot write based on a non-regular file: " + featurePath.toUri());
@droazen

droazen Apr 18, 2017 • edited

Contributor

Can you add a test case showing that the new exception is thrown in the non-regular file case?

@magicDGS

magicDGS Apr 23, 2017

Contributor

Done.

@droazen
Contributor

droazen commented Apr 18, 2017

Two comments, back to @magicDGS for changes

magicDGS was assigned by droazen Apr 18, 2017

magicDGS added some commits Apr 4, 2017

@magicDGS @magicDGS magicDGS Throw instead of silently failing for Index.writeBasedOnFeaturePath 558f52f
@magicDGS @magicDGS magicDGS Remove unused logger 647b21d
@magicDGS magicDGS Update javadoc b7e1495
@magicDGS magicDGS Add test to exercise the new behaviour

Some checks were not successful

1 failing and 3 successful checks
codecov/changes — 1 file has unexpected coverage changes not visible in diff.
Details
codecov/patch — 100% of diff hit (target 64.939%)
Details
codecov/project — 64.953% (+0.014%) compared to ee21d81
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
06074a5
@magicDGS magicDGS Fix import

Some checks were not successful

1 failing and 3 successful checks
codecov/changes — 2 files have unexpected coverage changes not visible in diff.
Details
codecov/patch — 100% of diff hit (target 64.939%)
Details
codecov/project — 64.953% (+0.014%) compared to ee21d81
Details
continuous-integration/travis-ci/pr — The Travis CI build passed
Details
9ca3846
@magicDGS
Contributor

magicDGS commented Apr 23, 2017

I've addressed the comments - back to you @droazen. Thanks for reviewing!
@lbergelson
Contributor

lbergelson commented May 16, 2017

@magicDGS The changes here look good. @yfarjoun wants to run this against the picard tests to make sure nothing was relying on the old behavior.

@lbergelson lbergelson assigned yfarjoun and unassigned magicDGS May 16, 2017

@yfarjoun
Contributor

yfarjoun commented May 27, 2017

not entirely sure what's going on, but when I build picard with this htsjdk I get the following error:

htsjdk.samtools.SAMException: Missing sequence header at line 1 in fastq /Users/farjoun/picard-private/Picard-public/testdata/picard/sam/fastq2bam/permissive-format/s_1_2_sequence.txt
	at htsjdk.samtools.fastq.FastqReader.readNextRecord(FastqReader.java:107)
	at htsjdk.samtools.fastq.FastqReader.<init>(FastqReader.java:93)
	at htsjdk.samtools.fastq.FastqReader.<init>(FastqReader.java:77)
	at picard.sam.FastqToSam.fileToFastqReader(FastqToSam.java:360)
	at picard.sam.FastqToSam.doWork(FastqToSam.java:251)
	at picard.cmdline.CommandLineProgram.instanceMain(CommandLineProgram.java:205)
	at picard.cmdline.PicardCommandLine.instanceMain(PicardCommandLine.java:94)
	at picard.cmdline.PicardCommandLine.instanceMain(PicardCommandLine.java:99)
@yfarjoun
Contributor

yfarjoun commented May 27, 2017

looks like the problem goes away with a rebase onto htsjdk/master, so nevermind.
@yfarjoun
Contributor

yfarjoun commented May 27, 2017

👍

@lbergelson lbergelson merged commit cc8a1a1 into samtools:master May 27, 2017
3 of 4 checks passed

3 of 4 checks passed

codecov/changes 2 files have unexpected coverage changes not visible in diff.
Details
codecov/patch 100% of diff hit (target 64.939%)
Details
codecov/project 64.953% (+0.014%) compared to ee21d81
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

magicDGS deleted the magicDGS:dgs_issue_821 branch May 28, 2017

@magicDGS
Contributor

magicDGS commented May 29, 2017

Thnaks! And sorry for the inconvenience of the rebasing issue!
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment