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!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
