-
Notifications
You must be signed in to change notification settings - Fork 245
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
Conversation
Inspired by review comments in #1219 which cover the use of |
There was a problem hiding this 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) { |
There was a problem hiding this comment.
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.
Codecov Report
@@ 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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