[ 
https://issues.apache.org/jira/browse/GROOVY-8520?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16425493#comment-16425493
 ] 

Remko Popma edited comment on GROOVY-8520 at 4/4/18 1:23 PM:
-------------------------------------------------------------

h3. Breakdown of failing tests in {{CliBuilderTest}}
h4. Behavioural Changes (please give feedback)
 * testTypedUnparsedFromInstance/testTypedUnparsedFromSpec: fails because the 
test tries to define an {{@Unparsed Integer[] nums}} field/method. The new 
CliBuilder impl. only allows field/methods of type {{String[]}} array or 
{{List}} to be annotated with {{@Unparsed}}. Picocli distinguishes between 
positional parameters (which can be typed) and "unmatched" input (list of 
strings). I propose to disallow strongly typed {{@Unparsed}} fields and add 
functionality for strongly typed positional parameters later (e.g. different 
types at different positions would then be possible). This would be different 
from previous 2.5 releases though. Thoughts?

 * testFailedParsePrintsUsage: the new CliBuilder introduces an {{errorWriter}} 
(default stderr) for error messages on invalid user input. Requested usage help 
goes to the existing {{printWriter}} (default stdout). This follows command 
line application conventions: diagnostic output should go to stderr to prevent 
it from being parsed when its output is used as input for another process. 
Requested help should always go to stdout so that users can pipe it to {{less}} 
or {{grep}}. This would be an incompatible change though. Thoughts?

 * testParseFromInstance: previous CliBuilder initialized annotated {{Boolean}} 
fields/setters to {{false}} even when they were not specified on the command 
line. Picocli does not modify options that are not specified. The latter seems 
more correct/natural to me but would be different behaviour from previous 2.5 
releases. Thoughts?

h4. Bugs that I may need help with 
 * testParseFromSpec: for some reason the map to interface coercion is not 
working... 
 * testParseFromInstanceFlagEdgeCases (same as above)

h4. Other bugs
 * testMultipleOccurrencesSeparateSeparate: to be backwards compatible, 
{{cli.a}} should return the first element of the values matched for the {{-a}} 
option, not the full list
 * testMultipleOccurrencesTogetherJuxtaposed (same as above)
 * testMultipleOccurrencesSeparateJuxtaposed (same as above)
 * testMultipleOccurrencesTogetherSeparate (same as above)
 * testMixedBurstingAndLongOptions: to be backwards compatible, {{longOpt}} 
names should be registered with both a single hyphen and with two hyphens in 
picocli.
 * testMultipleArgs: to be backwards compatible, {{args: 2}} (mapped to 
{{arity=2}} in picocli) needs to count option parameters _after_ the parameters 
have been split into parts.
 * testValSepClass: to be backwards compatible, {{args: 2}} should be mapped to 
{{arity="1..2"}} in picocli

h4. Invalid (test uses commons-cli API)
 * testLongAndShortOpts_allOptionsValid
 * testSampleLong
 * testSampleShort
 * testLongOptsOnly_nonOptionShouldStopArgProcessing

h4. Fixed
 * testConvertClass: fix by adding {{as CommandLine.ITypeConverter}} when 
installing the closure instance as type converter on the OptionSpec.Builder 
(CliBuilder line 497)
 * testParseFromInstance: fix by changing {{m.invoke(t, [it])}} to 
{{m.invoke(t, [it].toArray())}} at CliBuilder line 442, 550, 553 and 558

h3. TODO
 * Add tests for unparsed variations:
 ** {{@Unparsed String[] unmatched}} field
 ** {{@Unparsed List unmatched}} field
 ** {{@Unparsed List unmatched()}} method on spec interface and concrete 
instance
 ** {{@Unparsed unmatched(List)}} setter method on concrete instance
 ** {{@Unparsed unmatched(String[])}} setter method on concrete instance
 ** {{@Unparsed unmatched(String)}} setter (actually accumulator) method on 
concrete instance
 * other tests...


was (Author: rem...@yahoo.com):
h3. Breakdown of failing tests in {{CliBuilderTest}}
h4. Behavioural Changes (please give feedback)
 * testTypedUnparsedFromInstance/testTypedUnparsedFromSpec: fails because the 
test tries to define an {{@Unparsed Integer[] nums}} field/method. The new 
CliBuilder impl. only allows field/methods of type {{String[]}} array or 
{{List}} to be annotated with {{@Unparsed}}. Picocli distinguishes between 
positional parameters (which can be typed) and "unmatched" input (list of 
strings). I propose to disallow strongly typed {{@Unparsed}} fields and add 
functionality for strongly typed positional parameters later (e.g. different 
types at different positions would then be possible). This would be different 
from previous 2.5 releases though. Thoughts?

 * testFailedParsePrintsUsage: the new CliBuilder introduces an {{errorWriter}} 
(default stderr) for error messages on invalid user input. Requested usage help 
goes to the existing {{printWriter}} (default stdout). This follows command 
line application conventions: diagnostic output should go to stderr to prevent 
it from being parsed when its output is used as input for another process. 
Requested help should always go to stdout so that users can pipe it to {{less}} 
or {{grep}}. This would be an incompatible change though. Thoughts?

 * testParseFromInstance: previous CliBuilder initialized annotated {{Boolean}} 
fields/setters to {{false}} even when they were not specified on the command 
line. Picocli does not modify options that are not specified. The latter seems 
more correct/natural to me but would be different behaviour from previous 2.5 
releases. Thoughts?

h4. Bug
 * testParseFromSpec: for some reason the map to interface coercion is not 
working... {color:#FF0000}I may need help with this{color}
 * testParseFromInstanceFlagEdgeCases (same as above)
 * testMultipleOccurrencesSeparateSeparate: to be backwards compatible, 
{{cli.a}} should return the first element of the values matched for the {{-a}} 
option, not the full list
 * testMultipleOccurrencesTogetherJuxtaposed (same as above)
 * testMultipleOccurrencesSeparateJuxtaposed (same as above)
 * testMultipleOccurrencesTogetherSeparate (same as above)
 * testMixedBurstingAndLongOptions: to be backwards compatible, {{longOpt}} 
names should be registered with both a single hyphen and with two hyphens in 
picocli.
 * testMultipleArgs: to be backwards compatible, {{args: 2}} (mapped to 
{{arity=2}} in picocli) needs to count option parameters _after_ the parameters 
have been split into parts.
 * testValSepClass: to be backwards compatible, {{args: 2}} should be mapped to 
{{arity="1..2"}} in picocli

h4. Invalid (test uses commons-cli API)
 * testLongAndShortOpts_allOptionsValid
 * testSampleLong
 * testSampleShort
 * testLongOptsOnly_nonOptionShouldStopArgProcessing

h4. Fixed
 * testConvertClass: fix by adding {{as CommandLine.ITypeConverter}} when 
installing the closure instance as type converter on the OptionSpec.Builder 
(CliBuilder line 497)
 * testParseFromInstance: fix by changing {{m.invoke(t, [it])}} to 
{{m.invoke(t, [it].toArray())}} at CliBuilder line 442, 550, 553 and 558

h3. TODO
 * Add tests for unparsed variations:
 ** {{@Unparsed String[] unmatched}} field
 ** {{@Unparsed List unmatched}} field
 ** {{@Unparsed List unmatched()}} method on spec interface and concrete 
instance
 ** {{@Unparsed unmatched(List)}} setter method on concrete instance
 ** {{@Unparsed unmatched(String[])}} setter method on concrete instance
 ** {{@Unparsed unmatched(String)}} setter (actually accumulator) method on 
concrete instance
 * other tests...

> Replace commons-cli with picocli in CliBuilder
> ----------------------------------------------
>
>                 Key: GROOVY-8520
>                 URL: https://issues.apache.org/jira/browse/GROOVY-8520
>             Project: Groovy
>          Issue Type: Improvement
>          Components: command line processing
>            Reporter: Remko Popma
>            Priority: Major
>
> This ticket proposes to replace commons-cli with picocli in 
> {{groovy.util.CliBuilder}}.
> See [discussion on the mailing 
> list|https://lists.apache.org/thread.html/d60b6d5d4411e9ba0d7dc209cde8a9bb4abb00f0b9c0322f068c322e@%3Cdev.groovy.apache.org%3E]
>  for the original proposal and comparison with other CLI libraries.
> Goals for the initial implementation:
> * preserve the current CliBuilder behaviour as much as possible
> * deliver an implementation, tests and documentation in time to be included 
> in the 2.5 GA release



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to