mthmulders commented on a change in pull request #605:
URL: https://github.com/apache/maven/pull/605#discussion_r780672935
##########
File path: maven-embedder/src/test/java/org/apache/maven/cli/CLIManagerTest.java
##########
@@ -38,13 +40,19 @@ public void setup()
}
@Test
- public void spacedOptions()
+ public void spacedOptionsShort()
throws Exception
{
- CommandLine cmdLine = cliManager.parse( "-X -Dx=1 -D y=2 test".split(
" " ) );
+ CommandLine cmdLine = cliManager.parse( "-X -Dv -Dw=1 -D x=2
test".split( " " ) );
+
assertTrue( cmdLine.hasOption( CLIManager.VERBOSE ) );
- assertThat( cmdLine.getOptionValues( CLIManager.SET_SYSTEM_PROPERTY
)[0], is( "x=1" ) );
- assertThat( cmdLine.getOptionValues( CLIManager.SET_SYSTEM_PROPERTY
)[1], is( "y=2" ) );
- }
+ String[] properties = cmdLine.getOptionValues(
CLIManager.SET_SYSTEM_PROPERTY );
+ assertThat( properties[0], is( "v" ) );
+ assertThat( properties[1], is( "w" ) );
+ assertThat( properties[2], is( "1" ) );
+ assertThat( properties[3], is( "x" ) );
+ assertThat( properties[4], is( "2" ) );
+ assertThat( properties, not( hasItemInArray( "test" ) ) );
Review comment:
Correct. It doesn't make sense to have a test describe a Commons CLI
feature that we don't use. It would however duplicate `MavenCliTest`, so I
think it would make more sense to remove this one. Agreed?
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/CLIManager.java
##########
@@ -123,7 +123,7 @@ public CLIManager()
options = new Options();
options.addOption( Option.builder( Character.toString( HELP )
).longOpt( "help" ).desc( "Display help information" ).build() );
options.addOption( Option.builder( Character.toString(
ALTERNATE_POM_FILE ) ).longOpt( "file" ).hasArg().desc( "Force the use of an
alternate POM file (or directory with pom.xml)" ).build() );
- options.addOption( Option.builder( Character.toString(
SET_SYSTEM_PROPERTY ) ).hasArg().desc( "Define a system property" ).build() );
+ options.addOption( Option.builder( Character.toString(
SET_SYSTEM_PROPERTY ) ).numberOfArgs( 2 ).valueSeparator( '=' ).desc( "Define a
system property" ).build() );
Review comment:
No. The Commons CLI documentation says to do something different, but
doing that would "eat" the remaining arguments; as a result, "mvn -Dx=y verify"
would fail with "No goals have been specified for this build".
--
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]
