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

Fixes for issues found by SpotBugs #1283

Merged
merged 1 commit into from
Feb 20, 2019
Merged

Fixes for issues found by SpotBugs #1283

merged 1 commit into from
Feb 20, 2019

Conversation

jmthibault79
Copy link
Contributor

@jmthibault79 jmthibault79 commented Feb 14, 2019

Description

Resolves #1279 - removed the ExternalBlock class which was not truly necessary
Resolves #1280 - removed the unused CramIterator.nextRecord field
Resolves #1281 - removed the unused CramHeader.clone() method

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
Copy link

codecov-io commented Feb 14, 2019

Codecov Report

Merging #1283 into master will increase coverage by 0.006%.
The diff coverage is 78.571%.

@@               Coverage Diff               @@
##              master     #1283       +/-   ##
===============================================
+ Coverage     67.495%   67.501%   +0.006%     
+ Complexity      8150      8149        -1     
===============================================
  Files            558       557        -1     
  Lines          33364     33352       -12     
  Branches        5608      5607        -1     
===============================================
- Hits           22519     22513        -6     
+ Misses          8657      8651        -6     
  Partials        2188      2188
Impacted Files Coverage Δ Complexity Δ
...ava/htsjdk/samtools/cram/structure/CramHeader.java 48% <ø> (+8%) 7 <0> (ø) ⬇️
src/main/java/htsjdk/samtools/CRAMIterator.java 81.343% <0%> (+0.184%) 32 <0> (ø) ⬇️
...ain/java/htsjdk/samtools/cram/structure/Slice.java 60.248% <100%> (ø) 38 <0> (ø) ⬇️
...va/htsjdk/samtools/cram/structure/block/Block.java 75.362% <83.333%> (+0.362%) 23 <7> (+1) ⬆️

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

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

Looks good - requested a couple of tests.

Copy link
Collaborator

@cmnbroad cmnbroad left a comment

Choose a reason for hiding this comment

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

A couple of minor changes requested, otherwise looks good once these are addressed and tests pass again.

rm ExternalBlock class, add static factory method
- resolves use of initialized value found by SpotBugs

rm unused nextRecord

nextRecord -> samRecord

basic content ID check test

createRawNonExternalBlock and more tests

enableFileSourceTest

revert CRAMFileReaderTest

BlockTest to Block package

Update src/test/java/htsjdk/samtools/cram/structure/BlockTest.java

Co-Authored-By: jmthibault79 <[email protected]>
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