Legacy parser should throw on optional List argument if not provided. #99
Conversation
|
@cmnbroad can the optional annotation just be removed? |
| @@ -423,13 +423,16 @@ private boolean checkNumArguments() { | |||
| } | |||
| if (optionDefinition.isCollection) { | |||
| final Collection<?> c = (Collection<?>) optionDefinition.field.get(optionDefinition.parent); | |||
| if (c.size() == 0 && !optionDefinition.optional) { | |||
| messageStream.print("ERROR: Option '" + fullName + "' is required"); | |||
| return false; | |||
| @@ -423,13 +423,16 @@ private boolean checkNumArguments() { | |||
| } | |||
| if (optionDefinition.isCollection) { | |||
| final Collection<?> c = (Collection<?>) optionDefinition.field.get(optionDefinition.parent); | |||
| if (c.size() == 0 && !optionDefinition.optional) { | |||
| messageStream.print("ERROR: Option '" + fullName + "' is required"); | |||
| if (c.size() < optionDefinition.minElements) { | ||
| messageStream.println("ERROR: Option '" + fullName + "' must be specified at least " + | ||
| optionDefinition.minElements + " times."); | ||
| return false; | ||
| } | ||
| } else if (!optionDefinition.optional && !optionDefinition.hasBeenSet && | ||
| !optionDefinition.hasBeenSetFromParent && mutextOptionNames.length() == 0) { | ||
| } else if (!optionDefinition.optional && !optionDefinition.hasBeenSet && mutextOptionNames.length() == 0) { |
There was a problem hiding this comment.
mutextOptionNames isn't a collection, its a StringBuilder. No isEmpty on that one.
|
@nh13 and make everything required? I don't understand the suggestion. |
|
For non-collections, it's required if no value is set in the class. For collections, you use min/max elements anyhow. For options, you it's always optional (duh). |
|
@yfarjoun see a similar implementation that does away the need for the |
|
There are still a couple of problems with these changes - pls hold off reviewing until its ready (I made the PR a bit prematurely - I didn't think it would get reviewed so fast!). |
Codecov Report
@@ Coverage Diff @@
## master #99 +/- ##
============================================
+ Coverage 75.68% 75.69% +0.01%
Complexity 576 576
============================================
Files 22 22
Lines 2159 2160 +1
Branches 447 447
============================================
+ Hits 1634 1635 +1
- Misses 350 351 +1
+ Partials 175 174 -1
Continue to review full report at Codecov.
|
|
@nh13 We could get rid of the annotation, but then we'd have to propagate those changes to two parsers and all of the downstream projects.... |
|
@cmnbroad too bad we can't get rid of it, since it seems redundant. Your call, thanks for thinking about it. |
Fixes broadinstitute/picard#950.