[GitHub] commons-cli issue #25: CLI-284: Fix inconsistent behaviour for short and lon...
Github user sfuhrm commented on the issue: https://github.com/apache/commons-cli/pull/25 Hi @kinow thanks for your review! Oops, my maven build had a hickup and all tests were green. There's a design problem in the code you pointed me to. Every Option instance may be illegal (short and long == null) until first use. OptionBuilder (line 356) insists on creating an Option instance even if he long parameter is missing. Option.Builder does something similar. The setting happens after Option instance construction. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-cli issue #25: CLI-284: Fix inconsistent behaviour for short and lon...
Github user sfuhrm commented on the issue: https://github.com/apache/commons-cli/pull/25 Hi @kinow thanks for your review! Oops, my maven build had a hickup and all tests were green. There's a design problem in the code you pointed me to. Every Option instance may be illegal (short and long == null) until first use. OptionBuilder (line 356) insists on creating an Option instance even if he long parameter is missing. Option.Builder does something similar. The setting happens after Option instance construction. --- - To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
[GitHub] commons-cli issue #25: CLI-284: Fix inconsistent behaviour for short and lon...
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
[GitHub] commons-cli issue #25: CLI-284: Fix inconsistent behaviour for short and lon...
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