[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[GitHub] commons-cli issue #25: CLI-284: Fix inconsistent behaviour for short and lon...

Github user kinow commented on the issue:

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


To unsubscribe, e-mail: dev-unsubscribe@xxxxxxxxxxxxxxxxxx
For additional commands, e-mail: dev-help@xxxxxxxxxxxxxxxxxx