commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From kinow <...@git.apache.org>
Subject [GitHub] commons-cli issue #25: CLI-284: Fix inconsistent behaviour for short and lon...
Date Sun, 10 Jun 2018 00:02:56 GMT
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-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message