search

samtools / htsjdk

A Java API for high-throughput sequencing data (HTS) formats.
http://samtools.github.io/htsjdk/
283 stars 242 forks source link

Extract Allele into a interface with only one implementation as is currently the case. #1454

Closed vruano closed 3 years ago

vruano commented 4 years ago

This is the first of a few commits/PR to refactor Allele as intended in the "Large" stale PR https://github.com/samtools/htsjdk/pull/1370

This first commit extracts an interface from Allele that becomes the new Allele common . class ejem interface, and the sole implementation is ByteArrayAllele that as the name states, uses a byte[] array to encode the allele bases, symbolic ID etc like the original Allele class. This change was suggested by @droazen.

ByteArrayAllele is not mean to be use publicly (package protection level). To instantiate it you should use the original Allele static create methods.

It also does some deprecation marking of unused or smelly methods like the "wouldBe*" and acceptableBases families.

@lbergelson Please review.

mwalker174 commented 3 years ago

@vruano Were you planning to follow through on this? As the first step towards integrating symbolic alleles, it is very much needed for gatk-sv development.

codecov-io commented 3 years ago

Codecov Report

Merging #1454 (e84c648) into master (34881ae) will decrease coverage by 0.001%. The diff coverage is 73.276%.

@@               Coverage Diff               @@
##              master     #1454       +/-   ##
===============================================
- Coverage     69.418%   69.417%   -0.001%     
- Complexity      8940      8945        +5     
===============================================
  Files            604       605        +1     
  Lines          35632     35650       +18     
  Branches        5921      5928        +7     
===============================================
+ Hits           24735     24747       +12     
- Misses          8549      8557        +8     
+ Partials        2348      2346        -2
Impacted Files Coverage Δ Complexity Δ
...htsjdk/variant/variantcontext/ByteArrayAllele.java 69.136% <69.136%> (ø) 46.000 <46.000> (?)
.../htsjdk/variant/variantcontext/VariantContext.java 78.245% <71.429%> (-0.128%) 240.000 <3.000> (+2.000) :arrow_down:
...ain/java/htsjdk/variant/variantcontext/Allele.java 82.500% <85.714%> (+4.797%) 52.000 <1.000> (-43.000) :arrow_up:
...dk/samtools/util/SAMRecordPrefetchingIterator.java 74.667% <0.000%> (-1.333%) 13.000% <0.000%> (-1.000%)
src/main/java/htsjdk/samtools/SAMRecord.java 67.433% <0.000%> (-0.039%) 303.000% <0.000%> (ø%)
src/main/java/htsjdk/io/AsyncWriterPool.java 73.611% <0.000%> (+1.389%) 5.000% <0.000%> (ø%)
...htsjdk/samtools/util/nio/DeleteOnExitPathHook.java 90.476% <0.000%> (+9.524%) 4.000% <0.000%> (+1.000%)
vruano commented 3 years ago

@lbergelson addressed comments and it passes tests.

lbergelson commented 3 years ago

@vruano There's something about this branch that is making the diff show up with lots changes that are already in master. Have you rebased it recently?

vruano commented 3 years ago

Seems that the last rebase has fixed the issue with the additional master commit showing up. I never did anything different to the usual when rebasing so is kind of mystery to why it happened .... but now is gone :D

vruano commented 3 years ago

@lbergelson seems that the many-commit issues has dissipated by simply rebasing. Can you give it the ok and merge?