more changes to SAMTagUtil #1227

lbergelson · 2018-11-14T20:45:35Z

reverting the change from constants to static variables, deprecating
the constants instead

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)

* reverting the change from constants to static variables, deprecating the constants instead

droazen

Back to @lbergelson with minor comments.

droazen · 2018-11-14T20:48:34Z

src/main/java/htsjdk/samtools/SAMTagUtil.java

    /** @deprecated reserved tag for backwards compatibility only */
+


Stray whitespace separating this comment from the thing it's commenting on

droazen · 2018-11-14T20:49:38Z

src/main/java/htsjdk/samtools/SAMTagUtil.java

@@ -27,10 +27,12 @@

 /**
 * Facility for converting between String and short representation of a SAM tag.  short representation
- * is used by SAM JDK internally and is much more efficient.  Callers are encouraged to obtain the short
+ * is used by Htsjdk internally and is much more efficient.  Callers are encouraged to obtain the short


Htsjdk -> HTSJDK?

droazen · 2018-11-14T20:52:32Z

src/main/java/htsjdk/samtools/SAMTagUtil.java

 * value for a tag of interest once, and then use the SAMRecord attribute API that takes shorts rather than
 * Strings.
 *
+ * Tags that are defined by the SAM spec are included in the enum {@link SAMTag} along with their precomputed short tag.
+ *
 * @author alecw@broadinstitute.org
 */
 public final class SAMTagUtil {


You should perhaps consider deprecating this entire class, and moving the 2 non-deprecated methods to a different location (like the SAMTag enum)

codecov-io · 2018-11-14T21:39:13Z

Codecov Report

Merging #1227 into master will decrease coverage by 0.198%.
The diff coverage is 26.316%.

@@               Coverage Diff               @@
##              master     #1227       +/-   ##
===============================================
- Coverage     69.031%   68.832%   -0.198%     
+ Complexity      8077      8074        -3     
===============================================
  Files            539       539               
  Lines          32616     32617        +1     
  Branches        5510      5510               
===============================================
- Hits           22515     22451       -64     
- Misses          7894      7959       +65     
  Partials        2207      2207

Impacted Files	Coverage Δ	Complexity Δ
...c/main/java/htsjdk/samtools/sra/SRALazyRecord.java	`71.591% <0%> (ø)`	`130 <0> (ø)`	⬇️
src/main/java/htsjdk/samtools/SAMTagUtil.java	`0% <0%> (-98.649%)`	`0 <0> (-5)`
...ain/java/htsjdk/samtools/cram/structure/Slice.java	`44.545% <0%> (ø)`	`20 <0> (ø)`	⬇️
src/main/java/htsjdk/samtools/SAMTag.java	`97.368% <100%> (+0.353%)`	`5 <2> (+2)`	⬆️
...n/java/htsjdk/samtools/cram/structure/SliceIO.java	`88.889% <100%> (ø)`	`15 <0> (ø)`	⬇️
...ain/java/htsjdk/samtools/SAMBinaryTagAndValue.java	`84.884% <100%> (ø)`	`54 <0> (ø)`	⬇️
...n/java/htsjdk/samtools/cram/structure/ReadTag.java	`70.563% <100%> (ø)`	`65 <0> (ø)`	⬇️
...dk/samtools/SAMBinaryTagAndUnsignedArrayValue.java	`69.231% <100%> (ø)`	`5 <0> (ø)`	⬇️
...va/htsjdk/samtools/cram/digest/ContentDigests.java	`50.467% <50%> (ø)`	`9 <0> (ø)`	⬇️
src/main/java/htsjdk/samtools/SAMTextWriter.java	`66.292% <50%> (ø)`	`12 <0> (ø)`	⬇️
... and 3 more

lbergelson · 2018-11-15T20:49:21Z

@cmnbroad Could you take a look at this? I just deprecated the SamTagUtil entirely and moved all the functionality to SAMTag so that we wouldn't have this weird split between them.

cmnbroad · 2018-11-15T21:06:53Z

src/main/java/htsjdk/samtools/SAMTagUtil.java

@@ -140,8 +179,10 @@ public static SAMTagUtil getSingleton() {
     *
     * @param tag 2-character String representation of a tag name.
     * @return Tag name packed as 2 ASCII bytes in a short.
+     * @deprecated u


Forgot to finish the message....

woops, good catch

cmnbroad

One unfinished deprecation message that should be completed otherwise LGTM once tests pass.

pshapiro4broad

Looks good, just the usual collection of minor comments.

pshapiro4broad · 2018-11-15T21:32:49Z

src/main/java/htsjdk/samtools/SAMTag.java

-    private final short shortValue = SAMTag.makeBinaryTag(this.name());;
+    // Cache of already-converted tags.  Should speed up SAM text generation.
+    // Not synchronized because race condition is not a problem.
+    private static final String[] stringTags = new String[Short.MAX_VALUE];


static fields should be uppercase

Suggested change

private static final String[] stringTags = new String[Short.MAX_VALUE];

private static final String[] STRING_TAGS = new String[Short.MAX_VALUE];

This isn't really a constant though. My thought is that things that are constant are final, but this is a static cache that's updated with new values. It seems more confusing to make it caps since people assume those are immutable.

OK. I usually use all caps for any statics, since it's important to know what things are static, but I can see that all caps is typically used for constants only.

src/main/java/htsjdk/samtools/SAMTag.java

pshapiro4broad · 2018-11-15T21:43:24Z

src/main/java/htsjdk/samtools/SAMTagUtil.java

-        }
-        return ret;
+    @Deprecated
+    public String makeStringTag(final short tag) {


can be static

Suggested change

public String makeStringTag(final short tag) {

public static String makeStringTag(final short tag) {

Unless it needs to be non-static for backward compatibility?

Keeping it non-static for backwards compatibility. I was changing them to be static but I realized that would cause confusing breaks in binary compatibility.

This pr is basically me fixing some breaks in compatibility that I had though would be harmless but on further inspection seemd problematic.

pshapiro4broad · 2018-11-15T21:43:42Z

src/main/java/htsjdk/samtools/SAMTagUtil.java

     */
-    public static short makeBinaryTag(final String tag) {
+    @Deprecated
+    public short makeBinaryTag(final String tag) {


can be static

Suggested change

public short makeBinaryTag(final String tag) {

public static short makeBinaryTag(final String tag) {

Co-Authored-By: lbergelson <louisb@broadinstitute.org>

These comments were addressed.

lbergelson · 2018-11-16T15:38:04Z

@pshapiro4broad @cmnbroad Thank you for the reviews!

more changes to SAMTagUtil

db83cff

* reverting the change from constants to static variables, deprecating the constants instead

lbergelson assigned droazen Nov 14, 2018

lbergelson requested a review from droazen November 14, 2018 20:45

droazen previously requested changes Nov 14, 2018

View reviewed changes

more stuff

3e20504

lbergelson assigned cmnbroad and unassigned droazen Nov 15, 2018

lbergelson requested a review from cmnbroad November 15, 2018 20:49

cmnbroad reviewed Nov 15, 2018

View reviewed changes

cmnbroad approved these changes Nov 15, 2018

View reviewed changes

fix missed message

c2c19db

pshapiro4broad approved these changes Nov 15, 2018

View reviewed changes

Apply suggestions from code review

468cd3b

Co-Authored-By: lbergelson <louisb@broadinstitute.org>

lbergelson merged commit 698a4c3 into master Nov 16, 2018

lbergelson deleted the lb_more_mucking_with_samtagutils branch November 16, 2018 15:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

more changes to SAMTagUtil #1227

more changes to SAMTagUtil #1227

lbergelson commented Nov 14, 2018

droazen left a comment

droazen Nov 14, 2018

lbergelson Nov 15, 2018

droazen Nov 14, 2018

lbergelson Nov 15, 2018

droazen Nov 14, 2018 •

edited

Loading

lbergelson Nov 15, 2018

codecov-io commented Nov 14, 2018 •

edited

Loading

lbergelson commented Nov 15, 2018

cmnbroad Nov 15, 2018

lbergelson Nov 15, 2018

cmnbroad left a comment

pshapiro4broad left a comment

pshapiro4broad Nov 15, 2018

lbergelson Nov 16, 2018

pshapiro4broad Nov 16, 2018

pshapiro4broad Nov 15, 2018

lbergelson Nov 16, 2018

lbergelson Nov 16, 2018

pshapiro4broad Nov 15, 2018

lbergelson commented Nov 16, 2018

		/** @deprecated reserved tag for backwards compatibility only */

	private static final String[] stringTags = new String[Short.MAX_VALUE];
	private static final String[] STRING_TAGS = new String[Short.MAX_VALUE];

	public String makeStringTag(final short tag) {
	public static String makeStringTag(final short tag) {

	public short makeBinaryTag(final String tag) {
	public static short makeBinaryTag(final String tag) {

more changes to SAMTagUtil #1227

more changes to SAMTagUtil #1227

Conversation

lbergelson commented Nov 14, 2018

Checklist

droazen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

droazen Nov 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 14, 2018 • edited Loading

Codecov Report

lbergelson commented Nov 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmnbroad left a comment

Choose a reason for hiding this comment

pshapiro4broad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbergelson commented Nov 16, 2018

droazen Nov 14, 2018 •

edited

Loading

codecov-io commented Nov 14, 2018 •

edited

Loading