Skip to content

Fix #1049#1072

Merged
jacarey merged 3 commits intosamtools:masterfrom
danking:patch-1
Jan 29, 2018
Merged

Fix #1049#1072
jacarey merged 3 commits intosamtools:masterfrom
danking:patch-1

Conversation

@danking
Copy link
Contributor

@danking danking commented Jan 19, 2018

Description

Fixes #1049, return value of FastaSequenceIndexEntry.getIndexEntry is unusable.

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)

@danxmoran
Copy link

Hi @danking, you'll need to move FastaSequenceIndexEntry to its own file to get this to compile.

@pshapiro4broad
Copy link
Contributor

Hi @danking, you'll need to move FastaSequenceIndexEntry to its own file to get this to compile.

Given the way this class is used, making it a static inner class might be a better option. If the inner class was called Entry then you'd refer to it as FastaSequenceIndex.Entry.

@danking
Copy link
Contributor Author

danking commented Jan 24, 2018

@danxmoran hah sorry, too much time in Scala land.

I'll move it since that seems to be the lowest impact change.

@danking
Copy link
Contributor Author

danking commented Jan 24, 2018

@danxmoran I think this will compile.

@codecov-io
Copy link

codecov-io commented Jan 24, 2018

Codecov Report

Merging #1072 into master will not change coverage.
The diff coverage is 74.074%.

@@             Coverage Diff             @@
##              master     #1072   +/-   ##
===========================================
  Coverage     66.154%   66.154%           
- Complexity      7609      7618    +9     
===========================================
  Files            533       534    +1     
  Lines          32373     32373           
  Branches        5509      5508    -1     
===========================================
  Hits           21416     21416           
  Misses          8792      8792           
  Partials        2165      2165
Impacted Files Coverage Δ Complexity Δ
.../htsjdk/samtools/reference/FastaSequenceIndex.java 59.42% <ø> (-4.121%) 12 <0> (ø)
...dk/samtools/reference/FastaSequenceIndexEntry.java 74.074% <74.074%> (ø) 9 <9> (?)
...sjdk/samtools/util/Md5CalculatingOutputStream.java 69.231% <0%> (-7.692%) 8% <0%> (-1%)
...samtools/util/AsyncBlockCompressedInputStream.java 76% <0%> (+4%) 13% <0%> (+1%) ⬆️

Copy link

@danxmoran danxmoran left a comment

Choose a reason for hiding this comment

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

LGTM, over to @jacarey for another look & merge

@jacarey
Copy link
Contributor

jacarey commented Jan 25, 2018

Pinging @lbergelson @tfenne for objections to merge.

@lbergelson
Copy link
Member

👍

@jacarey jacarey merged commit b7ea382 into samtools:master Jan 29, 2018
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.

6 participants