Refactor MavenCli.populateRequest #351
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
rfscholte
reviewed
May 24, 2020
maven-core/src/main/java/org/apache/maven/execution/DefaultMavenExecutionRequest.java
Outdated
Show resolved
Hide resolved
rfscholte
reviewed
May 24, 2020
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
Outdated
Show resolved
Hide resolved
rfscholte
reviewed
May 24, 2020
maven-embedder/src/main/java/org/apache/maven/cli/MavenCli.java
Outdated
Show resolved
Hide resolved
|
Can't apply this patch anymore, please rebase |
mthmulders
force-pushed
the
refactor-mavencli-populaterequest
branch
from
1aa73fc
to
8ac313e
Compare
Just rebased against current state of master branch. |
…quest" This reverts commit 3c3ec2d.
This is a little more verbose code, but it makes sure flags in the ExecutionRequest will never be touched unless the flag is explicitly set.
mthmulders
force-pushed
the
refactor-mavencli-populaterequest
branch
from
8ac313e
to
a2e36c3
Compare
MartinKanters
approved these changes
Sep 19, 2020
There was a problem hiding this comment.
LGTM! @rfscholte were you done reviewing as well? I can merge it then tomorrow (or @mthmulders if he wants to)
rfscholte
approved these changes
Sep 20, 2020
|
Merged in ac80f5c. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While working on the resume feature (#342) I noticed that the
MavenCli#populateRequestmethod is very long and a bit unstructured. It follows all types of patterns for parsing and processing command line arguments and populating theMavenExecutionRequest.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:
determineProjectActivationanddetermineProfileActivationmethods are very much alike. We could merge them but it would lose quite some semantics, so I initially decided not to do so.determineLocalRepositoryPathonly uses information from theMavenExecutionRequestitself. I wonder if we could move it into theDefaultMavenExecutionRequest, but I'm unsure where / how exactly.Checklist
Make sure there is a JIRA issue 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.Format the pull request title like[MNG-XXX] - Fixes bug in ApproximateQuantiles, where you replaceMNG-XXXwith 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.mvn clean verifyto make sure basic checks pass. A more thorough check will be performed on your pull request automatically.I have signed the Individual Contributor License Agreement, and my employer signed the Corporate Contributor License Agreement.