Apply add dependency correctly, taking aggregating poms into account #4590
Conversation
…endencies when they are used as parent
rewrite-maven/src/main/java/org/openrewrite/maven/AddDependency.java
Outdated
Show resolved
Hide resolved
|
The naming between Not sure which one to refactor is any |
| @@ -115,6 +115,9 @@ public static int getModelVersion() { | |||
| @Builder.Default | |||
| List<Plugin> pluginManagement = emptyList(); | |||
|
|
|||
| @Builder.Default | |||
| List<String> modules = emptyList(); | |||
There was a problem hiding this comment.
To avoid confusion & future changes, perhaps it helps to already adopt the Maven 4 convention of calling these subprojects?
There was a problem hiding this comment.
Sounds good, that takes away the ambiguity!
There was a problem hiding this comment.
I've taken both modules and subProjects into account for the RawPom and named it subProjects anywhere beyond there
rewrite-maven/src/main/java/org/openrewrite/maven/tree/MavenResolutionResult.java
Outdated
Show resolved
Hide resolved
|
Perhaps some additions to the |
| if (!getResolutionResult().getPom().getSubprojects().isEmpty() | ||
| && (getResolutionResult().getModules().isEmpty() | ||
| || getResolutionResult().getModules().stream().map(MavenResolutionResult::getPom).map(ResolvedPom::getGav).map(ResolvedGroupArtifactVersion::getArtifactId).noneMatch(art -> getResolutionResult().getPom().getSubprojects().contains(art)) |
There was a problem hiding this comment.
The bot wants you to place the && and || on the previous line, in case that wasn't clear before.
Any why the use of Streams API in one of our most used recipes? Does it help to extract the condition into a properly named method such that you can do an early return from a loop there?
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
rewrite-maven/src/test/java/org/openrewrite/maven/internal/RawPomTest.java
Show resolved
Hide resolved
rewrite-maven/src/main/java/org/openrewrite/maven/tree/MavenResolutionResult.java
Outdated
Show resolved
Hide resolved
What's changed?
Introduce a mechanism to know which modules an aggregating pom applies to in order to prevent unnecessarily adding dependencies to aggregating poms
Anyone you would like to review specifically?
@timtebeek @sambsnyd
Any additional context
An aggregating pom could be a parent, when it's not there's no point in adding a dependency to it
Checklist