mthmulders commented on a change in pull request #605:
URL: https://github.com/apache/maven/pull/605#discussion_r749260193
##########
File path: maven-embedder/src/test/java/org/apache/maven/cli/CLIManagerTest.java
##########
@@ -41,10 +41,13 @@ public void setup()
public void spacedOptions()
throws Exception
{
- CommandLine cmdLine = cliManager.parse( "-X -Dx=1 -D y=2 test".split(
" " ) );
+ CommandLine cmdLine = cliManager.parse( "-X -Dx=1 -D y=2 -Dz
test".split( " " ) );
Review comment:
This test case is a bit strange.
In the "old" version, this test feeds `["-D", "y=2", "test"]` into the CLI
parsing code and expects Commons CLI to "understand" that `y=2` sets a property
(in other words, it is a value for the `-D` option), but `test` does not.
However, `cmdLine.getOptionValues( CLIManager.SET_SYSTEM_PROPERTY )`
contains `"test"` as well. It does so because we declared `-D` to allow
multiple args. `"y=2"` is the first argument, "test" the second. Commons CLI
[cannot tell the
difference](https://github.com/apache/commons-cli/blob/master/src/main/java/org/apache/commons/cli/DefaultParser.java#L497)
between `"y=2"` being an argument to `-D`, and `"test"` **not** being an
argument to `-D`: because we declared `-D` to have multiple arguments, it
doesn't know when the arguments stop.
In the "new" version, this does not happen. Why not? Because `-Dz` [is
detected as a new
option](https://github.com/apache/commons-cli/blob/master/src/main/java/org/apache/commons/cli/DefaultParser.java#L479),
with `"z"` as its first value. Commons CLI "closes" reading the previous `-D`.
It also
[assumes](https://github.com/apache/commons-cli/blob/master/src/main/java/org/apache/commons/cli/DefaultParser.java#L485)
that `"z"` is going to be the only argument for the new `-D` so it immediately
"closes" the last `-D` as well. `"test"` is then leftover, and interpreted as a
program argument.
The difficulty is that we've declared `-D` to have _multiple_ args. That,
together with the _value separator_ of `'='`, lets Commons CLI do the key/value
splitting. If we would say that `-D` has only _one_ arg for each time that `-D`
occurs (which is closer to what we expect), a lot of tests fail. Commons CLI
has [a helper method](n) to determine if a token is a Java property
declaration, but that method tests for _multiple_ argument values only.
---
The Java executable (`java(1)`) only accepts `"-Dfoo"` and `"-Dfoo=bar"`,
but it does not accept `"-D foo=whatever"` (note the space after `-D`) - hence
it also doesn't support `-D x=1 y=2`.
If we would express the above statement with Commons CLI, it would be
`Option.builder("D").hasArg().valueSeparator('=').build()` **but** that doesn't
work because Commons CLI only [treats options with **multiple** arguments as
possible Java property
style](https://github.com/apache/commons-cli/blob/master/src/main/java/org/apache/commons/cli/DefaultParser.java#L586).
I am tempted to consider this last thing a bug in Commons CLI, but I'm not
sure about it.
@michael-o, @rfscholte, what are your thoughts on this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
