Fix unbounded null throwing exception #41
Conversation
magicDGS
commented
Feb 10, 2017
magicDGS
commented
- Fixes Bug: null numeric unbounded arguments try thrown IllegalArgumentException #40
- Add previously failing test
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
===========================================
- Coverage 71.47% 71.4% -0.07%
Complexity 500 500
===========================================
Files 21 21
Lines 2040 2039 -1
Branches 421 420 -1
===========================================
- Hits 1458 1456 -2
Misses 405 405
- Partials 177 178 +1
Continue to review full report at Codecov.
|
|
Could you please have a look to this @cmnbroad? It is a very simple fix for a bug that is preventing me to update my dependencies to the latest GATK/Barclay versions... Thank you in advance. |
|
Hi @magicDGS Yes, will take a look tomorrow. |
|
Thank you very much @cmnbroad! |
| logger.warn("Argument --{} has value {}, but maximum recommended is {}", | ||
| argumentDefinition.getLongName(), argumentDoubleValue, argumentDefinition.maxRecommendedValue); | ||
| } | ||
| // if it does not have any recommended value, it does not log anything as expected |
There was a problem hiding this comment.
-> // if there is no recommended value, do not log anything
There was a problem hiding this comment.
I removed the comment and added an exception, because now it should not happen.
There was a problem hiding this comment.
Sorry, this should be the comment. I misunderstood where it is....
| public boolean hasBoundedRange() { | ||
| return this.minValue != Double.NEGATIVE_INFINITY || this.maxValue != Double.POSITIVE_INFINITY; | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this could be private for now. Also, now that this method exists, we should use it in the other places where we do the same check (i.e., in the argdef constructor). Likewise, we should add a hasRecommendedRange and use that at the appropriate call sites.
| // Integer without boundaries and null should be allowed | ||
| @Argument(doc = "Integer with null value allowed", optional = true) | ||
| public Integer nullInteger = null; | ||
| } |
There was a problem hiding this comment.
Its confusing to have this argument, and the corresponding test, live inside the "WithBoundariesArguments" class and test. It should live somewhere else, maybe in its own test if there is no current test thats appropriate. We should also add a similar test case for a Double (with null) arg.
|
|
||
| // Check hard limits first, if specified | ||
| if (isOutOfRange(argumentDefinition.minValue, argumentDefinition.maxValue, argumentDoubleValue)) { | ||
| if (argumentDefinition.hasBoundedRange() && isOutOfRange(argumentDefinition.minValue, argumentDefinition.maxValue, argumentDoubleValue)) { |
There was a problem hiding this comment.
We should add a hasRecommendedRange method, similar to hasBoundedRange, and use that below where we check the recommended range.
There was a problem hiding this comment.
It is not necessary for the logging path, because if there is no range there are no logging. Anyway, I included the check and added an exception (should never reach).
magicDGS
force-pushed
the
dgs_fix_unbounded_null
branch
from
faf867d to
dfc3196
Compare
|
I addressed your comments in the last commit. Thank you very much @cmnbroad -- back to you |
|
I think that this is ready to go, @cmnbroad. It is important for me to update dependencies with the new changes in some of my projects. Thank you in advance! |
|
Thanks for this! |
3 participants
