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

CRAM: Be explicit about spec IDs instead of using ordinal() #1221

Merged
merged 4 commits into from
Nov 6, 2018

Conversation

jmthibault79
Copy link
Contributor

@jmthibault79 jmthibault79 commented Nov 6, 2018

Description

Using ordinal() is brittle because code will break mysteriously if the orders of enums ever change. These Encoding, Block Compression Method, and BlockContentType IDs are mandated by the spec.

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)

@jmthibault79
Copy link
Contributor Author

Inspired by review comments in #1219 which cover the use of ordinal() for EncodingKey/DataSeries

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

@jmthibault79 Thank you, I have comment on naming and the inevitable request for more javadoc. 👍 to merge when those are addressed.

* The block compression methods specified by Section 8 of the CRAM spec
* @param value the number assigned to each block compression method in the CRAM spec
*/
BlockCompressionMethod(final int value) {
Copy link
Member

Choose a reason for hiding this comment

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

The naming of these is inconsistent and "spec" seems a bit strange in the name. I would drop spec and just go with getId or getTypeId as appropriate. The javadoc is a good addition, it would make sense to put it as class level javadoc and on the getter, as well since users will never actually see the enum constructor.

@lbergelson lbergelson assigned jmthibault79 and unassigned lbergelson and cmnbroad Nov 6, 2018
@codecov-io
Copy link

codecov-io commented Nov 6, 2018

Codecov Report

Merging #1221 into master will increase coverage by 0.097%.
The diff coverage is 95.122%.

@@               Coverage Diff               @@
##              master     #1221       +/-   ##
===============================================
+ Coverage     68.879%   68.976%   +0.097%     
- Complexity      8068      8118       +50     
===============================================
  Files            540       542        +2     
  Lines          32663     32852      +189     
  Branches        5527      5566       +39     
===============================================
+ Hits           22498     22660      +162     
- Misses          7968      7994       +26     
- Partials        2197      2198        +1
Impacted Files Coverage Δ Complexity Δ
...sjdk/samtools/cram/structure/BlockContentType.java 100% <100%> (ø) 3 <2> (+2) ⬆️
...ava/htsjdk/samtools/cram/structure/EncodingID.java 100% <100%> (ø) 3 <2> (+2) ⬆️
...ain/java/htsjdk/samtools/cram/structure/Block.java 62.5% <100%> (ø) 30 <0> (ø) ⬇️
...amtools/cram/structure/BlockCompressionMethod.java 100% <100%> (ø) 3 <2> (+2) ⬆️
...jdk/samtools/cram/structure/CompressionHeader.java 89.441% <100%> (ø) 28 <0> (ø) ⬇️
...k/samtools/cram/encoding/ByteArrayLenEncoding.java 67.241% <50%> (ø) 5 <0> (ø) ⬇️
...k/samtools/cram/encoding/ExternalIntegerCodec.java 43.75% <0%> (-14.583%) 3% <0%> (ø)
src/main/java/htsjdk/samtools/cram/io/LTF8.java 95.181% <0%> (-0.851%) 27% <0%> (+8%)
...encoding/huffman/codec/HuffmanIntegerEncoding.java 85.714% <0%> (ø) 10% <0%> (?)
...am/encoding/huffman/codec/HuffmanByteEncoding.java 43.75% <0%> (ø) 5% <0%> (?)
... and 3 more

Copy link
Member

@lbergelson lbergelson left a comment

Choose a reason for hiding this comment

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

👍

@lbergelson lbergelson merged commit e73d2f3 into master Nov 6, 2018
@lbergelson lbergelson deleted the jt_encoding_ordinal branch November 6, 2018 18:01
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.

4 participants