mthmulders commented on a change in pull request #605:
URL: https://github.com/apache/maven/pull/605#discussion_r742010235
##########
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:
That is covered in the (new) `MavenCliTest#populateProperties`. Should
we duplicate it here, then?
##########
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:
In the old situation, we used to do `cli.getOptionValue("-D")`. Given
arguments `"-Dx=1 -Dy -Dz=2"`, we would get `["x=1", "y", "z=2"]`. That's why
we did a `.split("=")` to separate the key from the value.
In the new situation, we do `cli.getOptions()`, then filter out those that
represent a "-D". Given arguments `"-Dx=1 -Dy -Dz=2"`, we would get **three**
`Option` instances:
1. where `getValues()` returns `["x", "1"]`
2. where `getValues()` returns `["y"]`
3. where `getValues()` returns `["z", "2"]`
This way we don't have to do magic with indices in the array anymore.
##########
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:
I'm fine with inlining it. I initially kept it as-is because of the
lengthy comment in there; I can't really judge whether it's (still) relevant.
##########
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:
This piece got replaced by a plain old for loop, as the extra checks
made the stream less obvious.
##########
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:
This piece got replaced by a plain old for loop, as the extra checks
made the stream less obvious.
By the way, the Common CLI javadoc mentions that the only reason for
`getOptionValues()` to return `null` is when the option is not defined - but
that was already checked the line before.
##########
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:
This piece got replaced by a plain old for loop, as the extra checks
made the stream less obvious.
By the way, the Common CLI Javadoc mentions that the only reason for
`getOptionValues()` to return `null` is when the option is not defined - but
that was already checked the line before.
##########
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:
1. The `option` variable is the result of `commandLine.getOptions()`.
The Javadoc for that method says it returns "processed Options". I would be
very disappointed if there were a `null` in that...
2. `--define w=1` doesn't work 😱 . Commons CLI seems to look for a `--define
w` option (which doesn't exist). I'll have to dig a little deeper why that is.
##########
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:
1. The `option` variable is the result of `commandLine.getOptions()`.
The Javadoc for that method says it returns "processed Options". I would be
very disappointed if there were a `null` in that...
2. `--define w=1` doesn't work 😱 . Commons CLI seems to look for a `--define
w` option (which doesn't exist). Commons CLI seems to think that if a "long
option" contains a `=` character, everything before that `=` is the name of the
option. That's not our case: we have a `" "` to separate the name from the
value, and the value then happens to contain a `=`.
##########
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:
That is covered in the (new) `MavenCliTest#populateProperties`. Should
we duplicate it here, then?
##########
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:
In the old situation, we used to do `cli.getOptionValue("-D")`. Given
arguments `"-Dx=1 -Dy -Dz=2"`, we would get `["x=1", "y", "z=2"]`. That's why
we did a `.split("=")` to separate the key from the value.
In the new situation, we do `cli.getOptions()`, then filter out those that
represent a "-D". Given arguments `"-Dx=1 -Dy -Dz=2"`, we would get **three**
`Option` instances:
1. where `getValues()` returns `["x", "1"]`
2. where `getValues()` returns `["y"]`
3. where `getValues()` returns `["z", "2"]`
This way we don't have to do magic with indices in the array anymore.
##########
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:
I'm fine with inlining it. I initially kept it as-is because of the
lengthy comment in there; I can't really judge whether it's (still) relevant.
##########
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:
This piece got replaced by a plain old for loop, as the extra checks
made the stream less obvious.
##########
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:
This piece got replaced by a plain old for loop, as the extra checks
made the stream less obvious.
By the way, the Common CLI javadoc mentions that the only reason for
`getOptionValues()` to return `null` is when the option is not defined - but
that was already checked the line before.
##########
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:
This piece got replaced by a plain old for loop, as the extra checks
made the stream less obvious.
By the way, the Common CLI Javadoc mentions that the only reason for
`getOptionValues()` to return `null` is when the option is not defined - but
that was already checked the line before.
##########
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:
1. The `option` variable is the result of `commandLine.getOptions()`.
The Javadoc for that method says it returns "processed Options". I would be
very disappointed if there were a `null` in that...
2. `--define w=1` doesn't work 😱 . Commons CLI seems to look for a `--define
w` option (which doesn't exist). I'll have to dig a little deeper why that is.
##########
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:
1. The `option` variable is the result of `commandLine.getOptions()`.
The Javadoc for that method says it returns "processed Options". I would be
very disappointed if there were a `null` in that...
2. `--define w=1` doesn't work 😱 . Commons CLI seems to look for a `--define
w` option (which doesn't exist). Commons CLI seems to think that if a "long
option" contains a `=` character, everything before that `=` is the name of the
option. That's not our case: we have a `" "` to separate the name from the
value, and the value then happens to contain a `=`.
--
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]
