michael-o commented on a change in pull request #605:
URL: https://github.com/apache/maven/pull/605#discussion_r741987540
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1759,27 +1757,8 @@ static void populateProperties( CommandLine commandLine,
Properties systemProper
systemProperties.setProperty( "maven.build.version", mavenBuildVersion
);
}
- private static void setCliProperty( String property, Properties properties
)
+ private static void setCliProperty( String name, String value, Properties
properties )
{
- String name;
-
- String value;
-
- int i = property.indexOf( '=' );
-
- if ( i <= 0 )
- {
- name = property.trim();
-
- value = "true";
- }
- else
- {
- name = property.substring( 0, i ).trim();
-
- value = property.substring( i + 1 );
- }
-
properties.setProperty( name, value );
Review comment:
Since there is only one line left, does it make sense to keep this
method at all if it is not used anywhere else?
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1730,18 +1731,15 @@ static void populateProperties( CommandLine
commandLine, Properties systemProper
// are most dominant.
//
----------------------------------------------------------------------
- if ( commandLine.hasOption( CLIManager.SET_SYSTEM_PROPERTY ) )
- {
- String[] defStrs = commandLine.getOptionValues(
CLIManager.SET_SYSTEM_PROPERTY );
-
- if ( defStrs != null )
- {
- for ( String defStr : defStrs )
+ Arrays.stream( commandLine.getOptions() )
+ .filter( option -> CLIManager.SET_SYSTEM_PROPERTY ==
option.getOpt().charAt( 0 ) )
+ .map( option -> option.getValues() )
+ .forEach( values ->
{
- setCliProperty( defStr, userProperties );
- }
- }
- }
+ final String key = values[0].trim();
Review comment:
I am bit confused how this magic works because it will deliver you a
consequitve array of even/odd indices representing a map actually because
`getValues()` results all values and not just value pairs.
##########
File path: maven-embedder/src/test/java/org/apache/maven/cli/CLIManagerTest.java
##########
@@ -43,8 +43,10 @@ public void spacedOptions()
{
CommandLine cmdLine = cliManager.parse( "-X -Dx=1 -D y=2 test".split(
" " ) );
Review comment:
Also add a test for `-Dz` (implicit `true`)
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1730,18 +1731,15 @@ static void populateProperties( CommandLine
commandLine, Properties systemProper
// are most dominant.
//
----------------------------------------------------------------------
- if ( commandLine.hasOption( CLIManager.SET_SYSTEM_PROPERTY ) )
- {
- String[] defStrs = commandLine.getOptionValues(
CLIManager.SET_SYSTEM_PROPERTY );
-
- if ( defStrs != null )
- {
- for ( String defStr : defStrs )
+ Arrays.stream( commandLine.getOptions() )
Review comment:
For consistency reasons with the rest I would retain the if clause.
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1731,15 +1730,19 @@ static void populateProperties( CommandLine
commandLine, Properties systemProper
// are most dominant.
//
----------------------------------------------------------------------
- Arrays.stream( commandLine.getOptions() )
- .filter( option -> CLIManager.SET_SYSTEM_PROPERTY ==
option.getOpt().charAt( 0 ) )
- .map( option -> option.getValues() )
- .forEach( values ->
- {
- final String key = values[0].trim();
- final String value = values.length == 2 ? values[1] :
"true";
- setCliProperty( key, value, userProperties );
- } );
+ for ( final Option option : commandLine.getOptions() )
+ {
+ final String opt = option.getOpt();
+ // Some options do not have a "short" version (e.g. color, debug).
+ // We are specifically looking for the "-D" option.
+ if ( opt != null && CLIManager.SET_SYSTEM_PROPERTY == opt.charAt(
0 ) )
Review comment:
Can the option be null action? What if the user has supplied `--define
...`?
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1759,27 +1757,8 @@ static void populateProperties( CommandLine commandLine,
Properties systemProper
systemProperties.setProperty( "maven.build.version", mavenBuildVersion
);
}
- private static void setCliProperty( String property, Properties properties
)
+ private static void setCliProperty( String name, String value, Properties
properties )
{
- String name;
-
- String value;
-
- int i = property.indexOf( '=' );
-
- if ( i <= 0 )
- {
- name = property.trim();
-
- value = "true";
- }
- else
- {
- name = property.substring( 0, i ).trim();
-
- value = property.substring( i + 1 );
- }
-
properties.setProperty( name, value );
Review comment:
Right, I have discussed this with @rfscholte. This will go away in
4.0.0. Keep as-is for now.
##########
File path: maven-embedder/src/test/java/org/apache/maven/cli/CLIManagerTest.java
##########
@@ -43,8 +43,10 @@ public void spacedOptions()
{
CommandLine cmdLine = cliManager.parse( "-X -Dx=1 -D y=2 test".split(
" " ) );
Review comment:
I would say yes for consistency.
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1730,18 +1731,15 @@ static void populateProperties( CommandLine
commandLine, Properties systemProper
// are most dominant.
//
----------------------------------------------------------------------
- if ( commandLine.hasOption( CLIManager.SET_SYSTEM_PROPERTY ) )
- {
- String[] defStrs = commandLine.getOptionValues(
CLIManager.SET_SYSTEM_PROPERTY );
-
- if ( defStrs != null )
- {
- for ( String defStr : defStrs )
+ Arrays.stream( commandLine.getOptions() )
+ .filter( option -> CLIManager.SET_SYSTEM_PROPERTY ==
option.getOpt().charAt( 0 ) )
+ .map( option -> option.getValues() )
+ .forEach( values ->
{
- setCliProperty( defStr, userProperties );
- }
- }
- }
+ final String key = values[0].trim();
Review comment:
Yes, now this perfectly makes sense. Thank you!
--
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]
