The existing code was throwing a NPE when using a CRAMFileReader for … #879

Merged
merged 2 commits into from May 30, 2017

Conversation

Projects
None yet
3 participants
Contributor

skashin commented May 25, 2017 edited

…InputResource.Type.PATH

Description

Please explain the changes you made here.
Explain the motivation for making this change. What existing problem does the pull request solve?

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)
@skashin skashin The existing code was throwing a NPE when using a CRAMFileReader for …
…InputResource.Type.PATH
701e70f

codecov-io commented May 25, 2017 edited

Codecov Report

Merging #879 into master will increase coverage by 0.007%.
The diff coverage is 67.241%.

@@               Coverage Diff               @@
##              master      #879       +/-   ##
===============================================
+ Coverage     65.006%   65.013%   +0.007%     
- Complexity      7229      7234        +5     
===============================================
  Files            528       528               
  Lines          31894     31895        +1     
  Branches        5444      5443        -1     
===============================================
+ Hits           20733     20736        +3     
- Misses          9013      9015        +2     
+ Partials        2148      2144        -4
Impacted Files Coverage Δ Complexity Δ
...rc/main/java/htsjdk/samtools/SamReaderFactory.java 61.809% <67.241%> (-1.456%) 7 <0> (ø)
...dk/samtools/seekablestream/SeekableFileStream.java 61.905% <0%> (-7.143%) 13% <0%> (-1%)
...sjdk/samtools/util/Md5CalculatingOutputStream.java 76.923% <0%> (-2.564%) 9% <0%> (ø)
.../main/java/htsjdk/tribble/index/AbstractIndex.java 52.91% <0%> (+1.331%) 31% <0%> (+1%) ⬆️
...c/main/java/htsjdk/samtools/fastq/FastqReader.java 82.432% <0%> (+1.351%) 26% <0%> (+2%) ⬆️
src/main/java/htsjdk/samtools/util/StringUtil.java 67.692% <0%> (+1.538%) 67% <0%> (+2%) ⬆️
...in/java/htsjdk/tribble/index/tabix/TabixIndex.java 78.788% <0%> (+1.679%) 39% <0%> (+1%) ⬆️
Contributor

yfarjoun commented May 27, 2017

Thanks for this.

this needs to be protected against future changes...can you please convert the list of if statements to a switch that includes a default case that will throw an exception? that should make things more clear when they break....

@skashin skashin Converted multiple if statements to a switch statement
68c3046
Contributor

skashin commented May 30, 2017

No problem, I switched away from the if statements

@yfarjoun

👍

Contributor

yfarjoun commented May 30, 2017

much better, thanks.

@yfarjoun yfarjoun merged commit 9bef3fc into samtools:master May 30, 2017

3 of 4 checks passed

codecov/changes 2 files have unexpected coverage changes not visible in diff.
Details
codecov/patch 67.241% of diff hit (target 65.006%)
Details
codecov/project 65.013% (+0.007%) compared to 1b27398
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yfarjoun yfarjoun added a commit that referenced this pull request May 31, 2017

@yfarjoun yfarjoun Revert "The existing code was throwing a NPE when using a CRAMFileRea…
…der for … (#879)"

This reverts commit 9bef3fc.
2c33e60

@yfarjoun yfarjoun added a commit that referenced this pull request May 31, 2017

@yfarjoun yfarjoun Revert "The existing code was throwing a NPE when using a CRAMFileRea…
…der for … (#879)" (#885)

This reverts commit 9bef3fc.
630aa48
Contributor

yfarjoun commented Jun 26, 2017

I guess you could revert the revert in a new branch and run the tests....or submit a PR and have travis test for you.

Contributor

skashin commented Jun 27, 2017

I ran "./gradlew test", and that built successfully.
What exactly did you mean by "somehow it broke the build"?

Contributor

yfarjoun commented Jun 27, 2017

@lbergelson can you help here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment