[
https://issues.apache.org/jira/browse/CLI-284?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16507206#comment-16507206
]
ASF GitHub Bot commented on CLI-284:
------------------------------------
Github user kinow commented on the issue:
https://github.com/apache/commons-cli/pull/25
Hi @sfuhrm thanks for the pull request. I had a look at the CLI-284 issue
in JIRA, and also checked out the pull request locally to take a look in
Eclipse.
I believe we have some tests failing due to this change. See the Travis-CI
build log, or try running `mvn clean test` locally. I got the following in my
environment:
```
Tests run: 410, Failures: 0, Errors: 55, Skipped: 54
```
The only other comment I have is about the duplicated code that we have now.
`Option`'s constructor checks for either `opt` or `longOpt` being present.
But so does `Option$Builder#build()`.
What about moving the
```java
if (opt == null && longOpt == null)
{
throw new IllegalArgumentException("Either opt or longOpt must be
specified");
}
```
check to `OptionValidator`? Maybe that way we avoid having the same if in
two places, and running the risk of someday changing one without changing the
other (though tests should catch it).
Thanks!
> Inconsistency in constraints for creating an instance of class Option using
> builder pattern or constructor
> ----------------------------------------------------------------------------------------------------------
>
> Key: CLI-284
> URL: https://issues.apache.org/jira/browse/CLI-284
> Project: Commons CLI
> Issue Type: Bug
> Reporter: Dilraj Singh
> Assignee: Bruno P. Kinoshita
> Priority: Major
> Attachments: CLI284.patch
>
>
> Builder pattern for creating an instance of class *Option* ensures that at
> least one of the representation (short and long representation) for the
> option is not null. It throws an *IllegalArgumentException* in-case program
> tries to build an instance with both the representations as null. Consider
> the following code snippet for reference.
> {code:java}
> public static void main(String[] args) {
> Option.Builder builder = Option.builder();
> Option op = builder.build();
> }
> {code}
> This piece of code fails with the following exception
> {noformat}
> Exception in thread "main" java.lang.IllegalArgumentException: Either opt or
> longOpt must be specified
> {noformat}
> But if we try to create an instance of Option by calling its constructor, It
> allows the creation with both opt and longOpt for Option as null. Consider
> the following code snippet for reference
> {code:java}
> public static void main(String[] args) {
> Option op = new Option(null, null);
> System.out.format("Short Representation: %s\n" +
> "Long Representation: %s", op.getOpt(),
> op.getLongOpt());
> }
> {code}
> Output:
> {noformat}
> Short Representation: null
> Long Representation: null
> {noformat}
> Calling a method on an instance with both opt and longOpt as null will lead
> to undesired null pointer exception. For example, calling
> {code:java}
> op.getId() {code}
> will throw a null pointer exception, thus violating the method contract.
> So as to fix this we need to make sure that whenever a constructor is invoked
> it has a non-null argument value for at least one of the Option
> representation.
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
