-
Notifications
You must be signed in to change notification settings - Fork 242
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
Updating SAMTag and SAMTagUtils #1208
Conversation
3d1a410
to
2e4dcf1
Compare
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.
Another approach would be to move the binary tag directly into the SAMTag
enum. e.g.
public enum SAMTag {
AM,
...
U2,
UQ;
public final short binaryTag;
SAMTag() {
binaryTag = makeBinaryTag(this.name());
}
}
You could also move makeStringTag()
, makeBinaryTag()
into SAMTag
, and delete SAMTagUtil
, or deprecate it and forward the methods and fields for now.
/** | ||
* This constructor is public despite being a utility class for backwards compatibility reasons. | ||
*/ | ||
public SAMTagUtil(){} |
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.
Shouldn't this be deprecated too?
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.
yes, good call
} else { | ||
encodedTag = tagCodec.encode(tagUtil.makeStringTag(attribute.tag), attribute.value); | ||
encodedTag = tagCodec.encode(SAMTagUtil.makeStringTag(attribute.tag), attribute.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.
Now that tagUtil
is unused it should be deleted.
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.
did this, good catch
@pshapiro4broad @magicDGS Thanks for the reviews. |
Codecov Report
@@ Coverage Diff @@
## master #1208 +/- ##
===============================================
+ Coverage 68.978% 69.037% +0.058%
- Complexity 8073 8092 +19
===============================================
Files 539 539
Lines 32587 32658 +71
Branches 5510 5513 +3
===============================================
+ Hits 22478 22546 +68
- Misses 7903 7905 +2
- Partials 2206 2207 +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.
Looks good to me
* adding new tags that were introduced in the spec * Cellular barcode tags s CB, CR, and CY added * Unique mollecular identifier tags BZ, MI, OX, QX, and RX added. * deprecating tag constants that are for backward compatibility only
* Making methods in SAMTagUtil final and deprecating the use of the getSingleton() method. * This is a breaking change to SAMTagUtil, it will break any subclasses, all other uses should be compatible
Co-Authored-By: lbergelson <[email protected]>
Co-Authored-By: lbergelson <[email protected]>
c198d9f
to
ef75df4
Compare
Description
This PR does several things: