search

samtools / htsjdk

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

There should be a constants class for file extensions #1382

Closed jb-adams closed 5 years ago

jb-adams commented 5 years ago

Solution for issue #1229

Moved file extension constants (for FASTQ, SAM/BAM/CRAM, VCFs) to a single file: IOExtensions so that they can be all referenced easily. Variables instantiated in original locations now import the file extension from IOExtensions, and @deprecated tags have been added to variables declared in original locations.

All tests passing locally

codecov-io commented 5 years ago

Codecov Report

Merging #1382 into master will decrease coverage by 0.023%. The diff coverage is 80%.

@@               Coverage Diff               @@
##              master     #1382       +/-   ##
===============================================
- Coverage     68.037%   68.014%   -0.023%     
+ Complexity      8364      8363        -1     
===============================================
  Files            571       572        +1     
  Lines          33880     33874        -6     
  Branches        5662      5662               
===============================================
- Hits           23051     23039       -12     
- Misses          8640      8644        +4     
- Partials        2189      2191        +2
Impacted Files Coverage Δ Complexity Δ
src/main/java/htsjdk/samtools/cram/CRAIIndex.java 88.571% <ø> (ø) 26 <0> (ø) :arrow_down:
...c/main/java/htsjdk/samtools/cram/build/CramIO.java 75.342% <ø> (ø) 20 <0> (ø) :arrow_down:
...c/main/java/htsjdk/samtools/util/IntervalList.java 72.18% <ø> (ø) 64 <0> (ø) :arrow_down:
src/main/java/htsjdk/tribble/util/TabixUtils.java 0% <ø> (ø) 0 <0> (ø) :arrow_down:
src/main/java/htsjdk/samtools/SBIIndex.java 59.091% <ø> (ø) 16 <0> (ø) :arrow_down:
src/main/java/htsjdk/variant/bcf2/BCF2Utils.java 74.757% <0%> (ø) 48 <0> (ø) :arrow_down:
src/main/java/htsjdk/samtools/BAMSBIIndexer.java 75% <0%> (ø) 1 <0> (ø) :arrow_down:
src/main/java/htsjdk/samtools/BamFileIoUtils.java 1.235% <0%> (-1.204%) 3 <0> (ø)
...mtools/reference/ReferenceSequenceFileFactory.java 73.684% <100%> (-5.039%) 22 <2> (ø)
.../main/java/htsjdk/tribble/readers/TabixReader.java 73.707% <100%> (ø) 66 <0> (ø) :arrow_down:
... and 21 more
jb-adams commented 5 years ago

not sure what happened here, does codecov raise an error if there's slightly lower coverage?

lbergelson commented 5 years ago

codecov is hyper sensitive. don't worry about that

yfarjoun commented 5 years ago

please change all the internal usages of the now deprecated code to use the new code.

jb-adams commented 5 years ago

spot bugs test failed, going through local report and will make changes

jb-adams commented 5 years ago

Thank you very much, happy to contribute! Is it ok for me to resolve these merge conflicts?

yfarjoun commented 5 years ago

please wait until one of the maintainers pipes in before rebaseing.

lbergelson commented 5 years ago

@jb-adams Could your rebase this and then we can merge? Thank you.

yfarjoun commented 5 years ago

hey @jb-adams,

Do you need help with rebasing and resolving the conflicts?

jb-adams commented 5 years ago

@yfarjoun @lbergelson sorry for the delay, I had a looming deadline for another project for the past couple days. I had a look at the conflicts earlier and should be ok to resolve them, am I ok to do so now?

jb-adams commented 5 years ago

ok, I've rebased and pushed the new branch. Looks like there are no merge conflicts, only the codecov/changes check failed.

lbergelson commented 5 years ago

@jb-adams Thank you. We can't really complain about others delay when we have PR's stuck in review for 6 months...