Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -535,11 +535,11 @@ 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)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add a hasRecommendedRange method, similar to hasBoundedRange, and use that below where we check the recommended range.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

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) {
Expand All @@ -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 there is no recommended value, do not log anything
}
}

Expand Down Expand Up @@ -1145,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()));
}
}
Expand Down Expand Up @@ -1186,6 +1184,15 @@ 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. */
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).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down