From 05ecbc9d8eefec8d88fe0bca9f139d86287feddc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20G=C3=B3mez-S=C3=A1nchez?= Date: Fri, 10 Feb 2017 18:57:44 +0100 Subject: [PATCH 1/3] Add failing test --- .../barclay/argparser/CommandLineArgumentParserTest.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java b/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java index 842fdd52..1c5c661c 100644 --- a/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java +++ b/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java @@ -841,6 +841,9 @@ public class WithBoundariesArguments { // recommended values are not explicitly verified by the tests, but do force the code through the warning code paths @Argument(doc = "Integer in the range [0, 30]", optional = true, minValue = 0, minRecommendedValue = 10, maxRecommendedValue = 15, maxValue = 30) public int integerArg = 20; + // Integer without boundaries and null should be allowed + @Argument(doc = "Integer with null value allowed", optional = true) + public Integer nullInteger = null; } @DataProvider @@ -853,7 +856,8 @@ public Object[][] withinBoundariesArgs() { {new String[]{"--doubleArg", "12"}, 12, 20}, {new String[]{"--doubleArg", "16"}, 16, 20}, {new String[]{"--doubleArg", "18"}, 18, 20}, - {new String[]{"--doubleArg", "20"}, 20, 20} + {new String[]{"--doubleArg", "20"}, 20, 20}, + {new String[]{"--nullInteger", "null"}, 15, 20} }; } @@ -864,6 +868,7 @@ public void testWithinBoundariesArguments(final String[] argv, final double expe Assert.assertTrue(clp.parseArguments(System.err, argv)); Assert.assertEquals(o.doubleArg, expectedDouble); Assert.assertEquals(o.integerArg, expectedInteger); + Assert.assertNull(o.nullInteger); } @DataProvider From bc833aec4010a65c084497c54776b0ad0fe7ab59 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20G=C3=B3mez-S=C3=A1nchez?= Date: Fri, 10 Feb 2017 19:02:03 +0100 Subject: [PATCH 2/3] Fix bug --- .../barclay/argparser/CommandLineArgumentParser.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java index 7d0dd41f..278b460f 100644 --- a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java +++ b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java @@ -535,7 +535,7 @@ private void checkArgumentRange(final ArgumentDefinition argumentDefinition, fin final Double argumentDoubleValue = (argumentValue == null) ? null : ((Number)argumentValue).doubleValue(); // Check hard limits first, if specified - if (isOutOfRange(argumentDefinition.minValue, argumentDefinition.maxValue, argumentDoubleValue)) { + if (argumentDefinition.hasBoundedRange() && isOutOfRange(argumentDefinition.minValue, argumentDefinition.maxValue, argumentDoubleValue)) { throw new CommandLineException.OutOfRangeArgumentValue(argumentDefinition.getLongName(), argumentDefinition.minValue, argumentDefinition.maxValue, argumentValue); } // Check recommended values @@ -552,6 +552,7 @@ private void checkArgumentRange(final ArgumentDefinition argumentDefinition, fin 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 } } @@ -1186,6 +1187,11 @@ public boolean isFlag(){ return field.getType().equals(boolean.class) || field.getType().equals(Boolean.class); } + /** Returns {@code true} if the argument has a bounded range; {@code false} otherwise. */ + public boolean hasBoundedRange() { + return this.minValue != Double.NEGATIVE_INFINITY || this.maxValue != Double.POSITIVE_INFINITY; + } + /** * Determine if this argument definition is controlled by a plugin (and thus subject to * descriptor dependency validation). From dfc319641b14140387291816b87c15989d6de88f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20G=C3=B3mez-S=C3=A1nchez?= Date: Fri, 17 Feb 2017 18:45:49 +0100 Subject: [PATCH 3/3] Responding to comments --- .../argparser/CommandLineArgumentParser.java | 15 +++--- .../CommandLineArgumentParserTest.java | 46 ++++++++++++++++--- 2 files changed, 48 insertions(+), 13 deletions(-) diff --git a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java index 278b460f..bc514cd4 100644 --- a/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java +++ b/src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java @@ -539,7 +539,7 @@ private void checkArgumentRange(final ArgumentDefinition argumentDefinition, fin throw new CommandLineException.OutOfRangeArgumentValue(argumentDefinition.getLongName(), argumentDefinition.minValue, argumentDefinition.maxValue, argumentValue); } // Check recommended values - if (isOutOfRange(argumentDefinition.minRecommendedValue, argumentDefinition.maxRecommendedValue, argumentDoubleValue)) { + if (argumentDefinition.hasRecommendedRange() && isOutOfRange(argumentDefinition.minRecommendedValue, argumentDefinition.maxRecommendedValue, argumentDoubleValue)) { final boolean outMinValue = argumentDefinition.minRecommendedValue != Double.NEGATIVE_INFINITY; final boolean outMaxValue = argumentDefinition.maxRecommendedValue != Double.POSITIVE_INFINITY; if (outMinValue && outMaxValue) { @@ -552,7 +552,7 @@ private void checkArgumentRange(final ArgumentDefinition argumentDefinition, fin 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 + // if there is no recommended value, do not log anything } } @@ -1146,10 +1146,7 @@ public ArgumentDefinition( // be set to an integer this.type = CommandLineParser.getUnderlyingType(this.field); if (! Number.class.isAssignableFrom(this.type)) { - if (this.maxValue != Double.POSITIVE_INFINITY - || this.minValue != Double.NEGATIVE_INFINITY - || this.maxRecommendedValue != Double.POSITIVE_INFINITY - || this.minRecommendedValue != Double.NEGATIVE_INFINITY) { + if (hasBoundedRange() || hasRecommendedRange()) { throw new CommandLineException.CommandLineParserInternalException(String.format("Min/max value ranges can only be set for numeric arguments. Argument --%s has a minimum or maximum value but has a non-numeric type.", this.getLongName())); } } @@ -1188,10 +1185,14 @@ public boolean isFlag(){ } /** Returns {@code true} if the argument has a bounded range; {@code false} otherwise. */ - public boolean hasBoundedRange() { + private boolean hasBoundedRange() { return this.minValue != Double.NEGATIVE_INFINITY || this.maxValue != Double.POSITIVE_INFINITY; } + private boolean hasRecommendedRange() { + return this.maxRecommendedValue != Double.POSITIVE_INFINITY || this.minRecommendedValue != Double.NEGATIVE_INFINITY; + } + /** * Determine if this argument definition is controlled by a plugin (and thus subject to * descriptor dependency validation). diff --git a/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java b/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java index 1c5c661c..107fa1d7 100644 --- a/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java +++ b/src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java @@ -805,6 +805,45 @@ private static String captureSystemStream(Runnable runnable, PrintStream stream, return out.toString(); } + + @CommandLineProgramProperties( + summary = "tool with nullable arguments", + oneLineSummary = "tools with nullable arguments", + programGroup = TestProgramGroup.class + ) + public class WithNullableArguments { + // Integer without boundaries and null should be allowed + @Argument(doc = "Integer with null value allowed", optional = true) + public Integer nullInteger = null; + // Double without boundaries and null should be allowed + @Argument(doc = "Double with null value allowed", optional = true) + public Double nullDouble= null; + } + + @DataProvider(name = "nullableArgs") + public Object[][] getNullableArguments() { + return new Object[][] { + // null values + {new String[]{}, null, null}, + {new String[]{"--nullInteger", "null"}, null, null}, + {new String[]{"--nullDouble", "null"}, null, null}, + {new String[]{"--nullInteger", "null", "--nullDouble", "null"}, null, null}, + // with values + {new String[]{"--nullInteger", "1"}, 1, null}, + {new String[]{"--nullDouble", "2"}, null, 2d}, + {new String[]{"--nullInteger", "1", "--nullDouble", "2"}, 1, 2.d}, + }; + } + + @Test(dataProvider = "nullableArgs") + public void testWithinBoundariesArguments(final String[] argv, final Integer expectedInteger, final Double expectedDouble) throws Exception { + final WithNullableArguments o = new WithNullableArguments(); + final CommandLineArgumentParser clp = new CommandLineArgumentParser(o); + Assert.assertTrue(clp.parseArguments(System.err, argv)); + Assert.assertEquals(o.nullInteger, expectedInteger); + Assert.assertEquals(o.nullDouble, expectedDouble); + } + @Test(expectedExceptions = CommandLineException.CommandLineParserInternalException.class) public void testWithBoundariesArgumentsForNoNumeric() { @CommandLineProgramProperties(summary = "broken tool", @@ -841,9 +880,6 @@ public class WithBoundariesArguments { // recommended values are not explicitly verified by the tests, but do force the code through the warning code paths @Argument(doc = "Integer in the range [0, 30]", optional = true, minValue = 0, minRecommendedValue = 10, maxRecommendedValue = 15, maxValue = 30) public int integerArg = 20; - // Integer without boundaries and null should be allowed - @Argument(doc = "Integer with null value allowed", optional = true) - public Integer nullInteger = null; } @DataProvider @@ -856,8 +892,7 @@ public Object[][] withinBoundariesArgs() { {new String[]{"--doubleArg", "12"}, 12, 20}, {new String[]{"--doubleArg", "16"}, 16, 20}, {new String[]{"--doubleArg", "18"}, 18, 20}, - {new String[]{"--doubleArg", "20"}, 20, 20}, - {new String[]{"--nullInteger", "null"}, 15, 20} + {new String[]{"--doubleArg", "20"}, 20, 20} }; } @@ -868,7 +903,6 @@ public void testWithinBoundariesArguments(final String[] argv, final double expe Assert.assertTrue(clp.parseArguments(System.err, argv)); Assert.assertEquals(o.doubleArg, expectedDouble); Assert.assertEquals(o.integerArg, expectedInteger); - Assert.assertNull(o.nullInteger); } @DataProvider