You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2020/05/24 10:37:50 UTC
[GitHub] [maven] mthmulders opened a new pull request #351: Refactor MavenCli.populateRequest
mthmulders opened a new pull request #351:
URL: https://github.com/apache/maven/pull/351
While working on the _resume_ feature (#342) I noticed that the [`MavenCli#populateRequest` method](https://github.com/apache/maven/blob/c6c7311713b0d60e445b2ffe32c5588791edf7ac/maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java#L1322) is very long and a bit unstructured. It follows all types of patterns for parsing and processing command line arguments and populating the `MavenExecutionRequest`.
I've refactored this method step-by-step. Hence there are a lot of small(ish) commits but I hope this makes it easier to review the changes and verify they don't break anything. The method itself is not easily testable so I couldn't rely on that. Where possible I created a few unit tests.
There are some bits & pieces left:
* The `determineProjectActivation` and `determineProfileActivation` methods are very much alike. We could merge them but it would lose quite some semantics, so I initially decided not to do so.
* The `determineLocalRepositoryPath` only uses information from the `MavenExecutionRequest` itself. I wonder if we could move it into the `DefaultMavenExecutionRequest`, but I'm unsure where / how exactly.
## Checklist
- [ ] ~~Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG) filed for the change (usually before you start working on it). Trivial changes like typos do not require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.~~
- [X] Each commit in the pull request should have a meaningful subject line and body.
- [ ] ~~Format the pull request title like `[MNG-XXX] - Fixes bug in ApproximateQuantiles`, where you replace `MNG-XXX` with the appropriate JIRA issue. Best practice is to use the JIRA issue title in the pull request title and in the first line of the commit message.~~
- [X] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [X] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully. **working on this**
I have signed the Individual Contributor License Agreement, and my employer signed the Corporate Contributor License Agreement.
[core-its]: https://maven.apache.org/core-its/core-it-suite/
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #351:
URL: https://github.com/apache/maven/pull/351#discussion_r429630233
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java
##########
@@ -687,6 +687,11 @@ public MavenExecutionRequest setPom( File pom )
{
this.pom = pom;
+ if ( this.pom != null && this.pom.getParentFile() != null )
Review comment:
IIRC this will go wrong. There are a couple of plugins that want to rewrite the pom (maven-shade-plugin, flatten-maven-plugin). This pom will be in the target directory, so it can be cleaned up. However, the basedir should stay the same.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on a change in pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #351:
URL: https://github.com/apache/maven/pull/351#discussion_r429637471
##########
File path: maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java
##########
@@ -687,6 +687,11 @@ public MavenExecutionRequest setPom( File pom )
{
this.pom = pom;
+ if ( this.pom != null && this.pom.getParentFile() != null )
Review comment:
Reverted that commit.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on a change in pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #351:
URL: https://github.com/apache/maven/pull/351#discussion_r491484783
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1331,103 +1335,172 @@ private MavenExecutionRequest populateRequest( CliRequest cliRequest )
return populateRequest( cliRequest, cliRequest.request );
}
- @SuppressWarnings( "checkstyle:methodlength" )
private MavenExecutionRequest populateRequest( CliRequest cliRequest, MavenExecutionRequest request )
{
+ slf4jLoggerFactory = LoggerFactory.getILoggerFactory();
CommandLine commandLine = cliRequest.commandLine;
String workingDirectory = cliRequest.workingDirectory;
boolean quiet = cliRequest.quiet;
- boolean showErrors = cliRequest.showErrors;
+ request.setShowErrors( cliRequest.showErrors ); // default: false
+ File baseDirectory = new File( workingDirectory, "" ).getAbsoluteFile();
- String[] deprecatedOptions = { "up", "npu", "cpu", "npr" };
- for ( String deprecatedOption : deprecatedOptions )
+ handleDeprecatedOptions( commandLine );
+
+ disableOnPresentOption( commandLine, CLIManager.BATCH_MODE, request::setInteractiveMode );
+ enableOnPresentOption( commandLine, CLIManager.SUPRESS_SNAPSHOT_UPDATES, request::setNoSnapshotUpdates );
Review comment:
Thanks for the suggestion. I just did that. Also, I rebased against the master branch to be sure the integration test would succeed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on a change in pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #351:
URL: https://github.com/apache/maven/pull/351#discussion_r429637493
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -19,6 +19,7 @@
* under the License.
*/
+import com.google.common.annotations.VisibleForTesting;
Review comment:
Replaced the annotation with a comment.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #351:
URL: https://github.com/apache/maven/pull/351#discussion_r429634094
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1318,103 +1322,165 @@ private MavenExecutionRequest populateRequest( CliRequest cliRequest )
return populateRequest( cliRequest, cliRequest.request );
}
- @SuppressWarnings( "checkstyle:methodlength" )
private MavenExecutionRequest populateRequest( CliRequest cliRequest, MavenExecutionRequest request )
{
+ slf4jLoggerFactory = LoggerFactory.getILoggerFactory();
CommandLine commandLine = cliRequest.commandLine;
String workingDirectory = cliRequest.workingDirectory;
boolean quiet = cliRequest.quiet;
- boolean showErrors = cliRequest.showErrors;
+ request.setShowErrors( cliRequest.showErrors ); // default: false
+ File baseDirectory = new File( workingDirectory, "" ).getAbsoluteFile();
- String[] deprecatedOptions = { "up", "npu", "cpu", "npr" };
- for ( String deprecatedOption : deprecatedOptions )
- {
- if ( commandLine.hasOption( deprecatedOption ) )
- {
- slf4jLogger.warn( "Command line option -{} is deprecated and will be removed in future Maven versions.",
- deprecatedOption );
- }
- }
+ warnAboutDeprecatedOptionsUsed( commandLine );
Review comment:
IMO the name is too long and too explicit. `handleDeprecatedOptions` should be sufficient and leaves enough room for changes, like breaking the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
mthmulders commented on pull request #351:
URL: https://github.com/apache/maven/pull/351#issuecomment-659619231
> Can't apply this patch anymore, please rebase
Just rebased against current state of master branch.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] MartinKanters commented on a change in pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #351:
URL: https://github.com/apache/maven/pull/351#discussion_r491362566
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1331,103 +1335,172 @@ private MavenExecutionRequest populateRequest( CliRequest cliRequest )
return populateRequest( cliRequest, cliRequest.request );
}
- @SuppressWarnings( "checkstyle:methodlength" )
private MavenExecutionRequest populateRequest( CliRequest cliRequest, MavenExecutionRequest request )
{
+ slf4jLoggerFactory = LoggerFactory.getILoggerFactory();
CommandLine commandLine = cliRequest.commandLine;
String workingDirectory = cliRequest.workingDirectory;
boolean quiet = cliRequest.quiet;
- boolean showErrors = cliRequest.showErrors;
+ request.setShowErrors( cliRequest.showErrors ); // default: false
+ File baseDirectory = new File( workingDirectory, "" ).getAbsoluteFile();
- String[] deprecatedOptions = { "up", "npu", "cpu", "npr" };
- for ( String deprecatedOption : deprecatedOptions )
+ handleDeprecatedOptions( commandLine );
+
+ disableOnPresentOption( commandLine, CLIManager.BATCH_MODE, request::setInteractiveMode );
+ enableOnPresentOption( commandLine, CLIManager.SUPRESS_SNAPSHOT_UPDATES, request::setNoSnapshotUpdates );
Review comment:
The old functionality sets the `noSnapshotUpdates` to false (or true if the option is there).
Because we do not set it now, it would become implicitly false (because it's a primitive boolean). Did you consider setting the default value explicitly?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
mthmulders commented on pull request #351:
URL: https://github.com/apache/maven/pull/351#issuecomment-695766670
Merged in ac80f5c.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
rfscholte commented on pull request #351:
URL: https://github.com/apache/maven/pull/351#issuecomment-647005421
Can't apply this patch anymore, please rebase
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rfscholte commented on a change in pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
rfscholte commented on a change in pull request #351:
URL: https://github.com/apache/maven/pull/351#discussion_r429633835
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -19,6 +19,7 @@
* under the License.
*/
+import com.google.common.annotations.VisibleForTesting;
Review comment:
This looks like a guava class. We want to avoid guava in our codebase, and only keep it as a transitive dependency for sisu(?). A comment explaining its visibility is good enough
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders commented on a change in pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
mthmulders commented on a change in pull request #351:
URL: https://github.com/apache/maven/pull/351#discussion_r429637535
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1318,103 +1322,165 @@ private MavenExecutionRequest populateRequest( CliRequest cliRequest )
return populateRequest( cliRequest, cliRequest.request );
}
- @SuppressWarnings( "checkstyle:methodlength" )
private MavenExecutionRequest populateRequest( CliRequest cliRequest, MavenExecutionRequest request )
{
+ slf4jLoggerFactory = LoggerFactory.getILoggerFactory();
CommandLine commandLine = cliRequest.commandLine;
String workingDirectory = cliRequest.workingDirectory;
boolean quiet = cliRequest.quiet;
- boolean showErrors = cliRequest.showErrors;
+ request.setShowErrors( cliRequest.showErrors ); // default: false
+ File baseDirectory = new File( workingDirectory, "" ).getAbsoluteFile();
- String[] deprecatedOptions = { "up", "npu", "cpu", "npr" };
- for ( String deprecatedOption : deprecatedOptions )
- {
- if ( commandLine.hasOption( deprecatedOption ) )
- {
- slf4jLogger.warn( "Command line option -{} is deprecated and will be removed in future Maven versions.",
- deprecatedOption );
- }
- }
+ warnAboutDeprecatedOptionsUsed( commandLine );
Review comment:
Accepted naming suggestion.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] MartinKanters commented on a change in pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
MartinKanters commented on a change in pull request #351:
URL: https://github.com/apache/maven/pull/351#discussion_r491362566
##########
File path: maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
##########
@@ -1331,103 +1335,172 @@ private MavenExecutionRequest populateRequest( CliRequest cliRequest )
return populateRequest( cliRequest, cliRequest.request );
}
- @SuppressWarnings( "checkstyle:methodlength" )
private MavenExecutionRequest populateRequest( CliRequest cliRequest, MavenExecutionRequest request )
{
+ slf4jLoggerFactory = LoggerFactory.getILoggerFactory();
CommandLine commandLine = cliRequest.commandLine;
String workingDirectory = cliRequest.workingDirectory;
boolean quiet = cliRequest.quiet;
- boolean showErrors = cliRequest.showErrors;
+ request.setShowErrors( cliRequest.showErrors ); // default: false
+ File baseDirectory = new File( workingDirectory, "" ).getAbsoluteFile();
- String[] deprecatedOptions = { "up", "npu", "cpu", "npr" };
- for ( String deprecatedOption : deprecatedOptions )
+ handleDeprecatedOptions( commandLine );
+
+ disableOnPresentOption( commandLine, CLIManager.BATCH_MODE, request::setInteractiveMode );
+ enableOnPresentOption( commandLine, CLIManager.SUPRESS_SNAPSHOT_UPDATES, request::setNoSnapshotUpdates );
Review comment:
The old functionality sets the `noSnapshotUpdates` to false (or true if the option is there).
Because we do not set it now, it would become implicitly false (because it's a primitive boolean). Did you consider setting the default value explicitly in the MavenExecutionRequest field? It's done for some other fields as well.
I think it will improve understandability, as people won't have to reason whether the boolean is a primitive and what the default would be etc.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] mthmulders closed pull request #351: Refactor MavenCli.populateRequest
Posted by GitBox <gi...@apache.org>.
mthmulders closed pull request #351:
URL: https://github.com/apache/maven/pull/351
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org