search

samtools / htsjdk

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

Rename LogLevel.WARNING to LogLevel.WARN. #1262

Closed heuermh closed 5 years ago

heuermh commented 5 years ago

Fixes #1261

codecov-io commented 5 years ago

Codecov Report

Merging #1262 into master will decrease coverage by 0.009%. The diff coverage is 66.667%.

@@               Coverage Diff               @@
##              master     #1262       +/-   ##
===============================================
- Coverage     69.353%   69.344%   -0.009%     
+ Complexity      8301      8300        -1     
===============================================
  Files            555       555               
  Lines          33116     33116               
  Branches        5572      5572               
===============================================
- Hits           22967     22964        -3     
- Misses          7890      7892        +2     
- Partials        2259      2260        +1
Impacted Files Coverage Δ Complexity Δ
src/main/java/htsjdk/samtools/util/Log.java 59.016% <66.667%> (ø) 17 <1> (ø) :arrow_down:
...samtools/util/AsyncBlockCompressedInputStream.java 72% <0%> (-4%) 12% <0%> (-1%)
heuermh commented 5 years ago

The code coverage report is wrong, there is no change to unit test coverage in this pull request.

heuermh commented 5 years ago

Ping @lbergelson for review. This is helpful for Disq and ADAM and other downstream tools.

lbergelson commented 5 years ago

@heuermh Sorry, lots of stuff built up here that I haven't gotten a chance to review yet.

This is a breaking change that would likely cause a lot of people to have to patch their source code so I want to understand the need for it.

I'm not sure I understand the benefit of this. If this is renamed will it enable automatic integration with slf4j? Is it just to standardize the language? Log4j / slf4j use WARN, but java.util.logging defines WARNING so it's not exactly an abberation.

lbergelson commented 5 years ago

We could do this rename seemlessly without breaking anyone by deprecating the WARNING level and then making the log priority explicit instead of the ordinal. Then we can remove uses of WARN in htsjdk itself without affecting downstream projects that rely on this enum.

yfarjoun commented 5 years ago

closing in lieu of #1270. please reopen if that is not a good replacement.