Skip to content

Fix unbounded null throwing exception #41

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Feb 27, 2017
Merged

Fix unbounded null throwing exception #41

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
05ecbc9
Add failing test
magicDGS Feb 10, 2017
bc833ae
Fix bug
magicDGS Feb 10, 2017
dfc3196
Responding to comments
magicDGS Feb 17, 2017
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
8 changes: 7 additions & 1 deletion src/main/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Collaborator

@cmnbroad cmnbroad Feb 17, 2017

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

@magicDGS magicDGS Feb 17, 2017

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
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 it does not have any recommended value, it does not log anything as expected
Copy link
Collaborator

@cmnbroad cmnbroad Feb 17, 2017

Choose a reason for hiding this comment

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

-> // if there is no recommended value, do not log anything

Copy link
Contributor Author

@magicDGS magicDGS Feb 17, 2017

Choose a reason for hiding this comment

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

I removed the comment and added an exception, because now it should not happen.

Copy link
Contributor Author

@magicDGS magicDGS Feb 17, 2017

Choose a reason for hiding this comment

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

Sorry, this should be the comment. I misunderstood where it is....

Copy link
Contributor Author

@magicDGS magicDGS Feb 17, 2017

Choose a reason for hiding this comment

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

Done.

}
}

Expand Down Expand Up @@ -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;
}

Copy link
Collaborator

@cmnbroad cmnbroad Feb 17, 2017

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@magicDGS magicDGS Feb 17, 2017

Choose a reason for hiding this comment

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

Done.

/**
* Determine if this argument definition is controlled by a plugin (and thus subject to
* descriptor dependency validation).
Expand Down
7 changes: 6 additions & 1 deletion src/test/java/org/broadinstitute/barclay/argparser/CommandLineArgumentParserTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Copy link
Collaborator

@cmnbroad cmnbroad Feb 17, 2017

Choose a reason for hiding this comment

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

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.


@DataProvider
Expand All @@ -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}
};
}

Expand All @@ -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
Expand Down