Yf add unknown so #862

yfarjoun · Apr 20, 2017

Description

Adds validation to the values in the SO and GO tags in the SAM header.

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)

codecov-io · Apr 20, 2017

Codecov Report

Merging #862 into master will increase coverage by 0.041%.
The diff coverage is 77.551%.

@@              Coverage Diff               @@
##              master     #862       +/-   ##
==============================================
+ Coverage     64.939%   64.98%   +0.041%     
- Complexity      7210     7283       +73     
==============================================
  Files            527      527               
  Lines          31802    32025      +223     
  Branches        5426     5486       +60     
==============================================
+ Hits           20652    20810      +158     
- Misses          9017     9082       +65     
  Partials        2133     2133

Impacted Files	Coverage Δ	Complexity Δ
.../main/java/htsjdk/samtools/SAMTextHeaderCodec.java	`80.579% <100%> (+1.268%)`	`46 <0> (+2)`	⬆️
.../main/java/htsjdk/samtools/SAMValidationError.java	`95.745% <100%> (+0.046%)`	`9 <0> (ø)`	⬇️
src/main/java/htsjdk/samtools/SAMFileHeader.java	`60.123% <71.053%> (+0.939%)`	`35 <8> (+1)`	⬆️
src/main/java/htsjdk/samtools/util/IOUtil.java	`32.578% <0%> (+1.633%)`	`73% <0%> (+13%)`	⬆️
...c/main/java/htsjdk/samtools/util/IntervalList.java	`68.276% <0%> (+3.364%)`	`114% <0%> (+56%)`	⬆️
...sjdk/samtools/util/Md5CalculatingOutputStream.java	`79.487% <0%> (+7.692%)`	`9% <0%> (+1%)`	⬆️

tfenne · Apr 20, 2017

tfenne reviewed Apr 20, 2017

View changes

Some fairly minor suggestions @yfarjoun

Well, if you're going to start cleaning up, this could be CollectionUtil.makeSet(....)

While it's probably not important, it's good practice to add new enum values at the end, in case anyone relied on the previous ordering/ordinal values.

Could hoist back onto the prior line now.

Should probably get rid of the so.equals("unknown") here since that's now a valid value?

The existing implementation here is poor - the fact that it translates from the String on every access. I hadn't realized it was doing that. This also means though, that it's going to log your warning on every access. What do you think about either changing the class to cache the SortOrder enum value or adding setAttribute("SO", "unknown") in the catch?

I don't like changing the header, as that might have side-effects (the header might be used later to write a file.)

I'll try to cache the resulting enum.

yfarjoun · May 2, 2017

merging under the assumption that the minor comments were addressed in my latest PR. @tfenne please let me know if I was being overly presumptuous and I'll be happy to revert and discuss further.

yfarjoun	`- added a "unknown" enum value to the SortOrder enum. It behaves exac…` …tly like unsorted, but the sam-spec allows for it. - wrapped getSortOrder() with a try-catch to translate any unknown values to 'unknown'		`3418ea0`
yfarjoun	`- added an error for non-conforming tags.` - added to the validation a check for both SO and GO to see that they are legal values.		`cb0fe30`
yfarjoun	`- aesthetic changes`		`0e1d1b1`
yfarjoun	`- lowercased variable names`		`460912d`
yfarjoun	`- added tests covering SO and GO tags`		`7013231`
yfarjoun	`- fixed a test that was broken for a while...`	Some checks were not successful 1 failing and 4 successful checks codecov/changes — 2 files have unexpected coverage changes not visible in diff. Details codecov/patch — 88.889% of diff hit (target 64.939%) Details codecov/project — 64.975% (+0.035%) compared to ee21d81 Details continuous-integration/travis-ci/pr — The Travis CI build passed Details continuous-integration/travis-ci/push — The Travis CI build passed Details	`e0b7ef2`

@@ -48,13 +49,14 @@
          public static final String GROUP_ORDER_TAG = "GO";
          public static final String CURRENT_VERSION = "1.5";
          public static final Set<String> ACCEPTABLE_VERSIONS =
-    -            new HashSet<String>(Arrays.asList("1.0", "1.3", "1.4", "1.5"));
+    +            new HashSet<>(Arrays.asList("1.0", "1.3", "1.4", "1.5"));

          private final Map<String, SAMReadGroupRecord> mReadGroupMap =
-    -        new HashMap<String, SAMReadGroupRecord>();
-    -    private final Map<String, SAMProgramRecord> mProgramRecordMap = new HashMap<String, SAMProgramRecord>();
+    +            new HashMap<>();

+    +        try {
+    +            return SortOrder.valueOf(so);
+    +        } catch (IllegalArgumentException e) {
+    +            log.warn("Found non conforming header SO tag: "+ so + ". Treating as 'unknown'.");

samtools/htsjdk

Yf add unknown so #862

yfarjoun commented Apr 20, 2017

yfarjoun added some commits Apr 20, 2017

codecov-io commented Apr 20, 2017 • edited

tfenne reviewed Apr 20, 2017

View changes

yfarjoun commented May 2, 2017

yfarjoun merged commit `ad8aa02` into master May 2, 2017
4 of 5 checks passed

4 of 5 checks passed

		unsorted(null),
		+ unknown(null),

		@@ -253,7 +255,12 @@ public SortOrder getSortOrder() {
		if (so == null \|\| so.equals("unknown")) {

samtools/htsjdk

Join GitHub today

Yf add unknown so #862

Conversation

yfarjoun commented Apr 20, 2017

Description

yfarjoun added some commits Apr 20, 2017

Some checks were not successful

codecov-io commented Apr 20, 2017 • edited

Codecov Report

tfenne reviewed Apr 20, 2017 View changes

tfenne Apr 20, 2017

tfenne Apr 20, 2017

tfenne Apr 20, 2017

tfenne Apr 20, 2017

tfenne Apr 20, 2017

yfarjoun Apr 21, 2017

Some checks were not successful

yfarjoun commented May 2, 2017

Hide details View details yfarjoun merged commit ad8aa02 into master May 2, 2017 4 of 5 checks passed

4 of 5 checks passed

tfenne reviewed Apr 20, 2017

View changes

yfarjoun merged commit `ad8aa02` into master May 2, 2017
4 of 5 checks passed