gnodet-bot commented on code in PR #13241:
URL: https://github.com/apache/maven/pull/13241#discussion_r4080948630
##########
impl/maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java:
##########
@@ -141,6 +140,7 @@ private Result<ProjectDependencyGraph>
reactorDependencyGraph(MavenSession sessi
trimSelectedProjects(activeProjects, allSortedProjects,
projectDependencyGraph, session.getRequest());
activeProjects = trimResumedProjects(activeProjects,
projectDependencyGraph, session.getRequest());
activeProjects = trimExcludedProjects(activeProjects,
projectDependencyGraph, session.getRequest());
+ validateExtensionIsNotPartOfReactor(activeProjects, projects,
session.getRequest());
Review Comment:
⚠️ **Naming not addressed** — `gnodet` suggested renaming this to
`validateReactorExtensionNotUsedInActiveReactor` (more accurately describes
what the method does) and the first parameter to `activeProjects`.
```suggestion
validateReactorExtensionNotUsedInActiveReactor(activeProjects,
projects, session.getRequest());
```
##########
impl/maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java:
##########
@@ -376,15 +376,17 @@ private List<MavenProject>
getProjectsForMavenReactor(MavenSession session) thro
return requestPomCollectionStrategy.collectProjects(request);
}
- private void validateProjects(List<MavenProject> projects,
MavenExecutionRequest request)
+ private void validateExtensionIsNotPartOfReactor(
+ List<MavenProject> projects, List<MavenProject> allProjects,
MavenExecutionRequest request)
Review Comment:
⚠️ **Naming not addressed** — same rename here per `gnodet`'s suggestion.
Also rename first parameter `projects` → `activeProjects` so the signature
matches the call site and documents the semantic difference from `allProjects`.
```suggestion
private void validateReactorExtensionNotUsedInActiveReactor(
List<MavenProject> activeProjects, List<MavenProject>
allProjects, MavenExecutionRequest request)
```
##########
impl/maven-core/src/main/java/org/apache/maven/graph/DefaultGraphBuilder.java:
##########
@@ -376,15 +376,17 @@ private List<MavenProject>
getProjectsForMavenReactor(MavenSession session) thro
return requestPomCollectionStrategy.collectProjects(request);
}
- private void validateProjects(List<MavenProject> projects,
MavenExecutionRequest request)
+ private void validateExtensionIsNotPartOfReactor(
+ List<MavenProject> projects, List<MavenProject> allProjects,
MavenExecutionRequest request)
throws MavenExecutionException {
Map<String, MavenProject> projectsMap = new HashMap<>();
- List<MavenProject> projectsInRequestScope =
getProjectsInRequestScope(request, projects);
+ List<MavenProject> projectsInRequestScope =
getProjectsInRequestScope(request, allProjects);
for (MavenProject p : projectsInRequestScope) {
- String projectKey = ArtifactUtils.key(p.getGroupId(),
p.getArtifactId(), p.getVersion());
-
- projectsMap.put(projectKey, p);
+ if (projects.contains(p)) {
+ String projectKey = ArtifactUtils.key(p.getGroupId(),
p.getArtifactId(), p.getVersion());
+ projectsMap.put(projectKey, p);
+ }
}
Review Comment:
⚠️ **Unnecessary indirection.** `getProjectsInRequestScope(request,
allProjects)` returns a scope-filtered subset of `allProjects`, and then
`projects.contains(p)` filters that subset down to only what is already in
`activeProjects`. The result is exactly `activeProjects` (possibly minus
projects added via `--also-make` that are not in request scope — an unintended
narrowing). Seed `projectsMap` directly from `projects` (now `activeProjects`):
```suggestion
for (MavenProject p : activeProjects) {
String projectKey = ArtifactUtils.key(p.getGroupId(),
p.getArtifactId(), p.getVersion());
projectsMap.put(projectKey, p);
}
```
This also drops the now-unused `allProjects` parameter (and the
`List<MavenProject> projectsInRequestScope` variable), so the method signature
simplifies back to `(List<MavenProject> activeProjects, MavenExecutionRequest
request)` — no `allProjects` needed at all.
##########
its/core-it-suite/src/test/java/org/apache/maven/it/MavenITmng5572ReactorPluginExtensionsTest.java:
##########
@@ -57,5 +57,16 @@ public void testit() throws Exception {
verifier.verifyErrorFreeLog();
verifier.verifyTextInLog(
"[WARNING] 'project' uses
'org.apache.maven.its.mng5572:plugin' as extension which is not possible within
the same reactor build. This plugin was pulled from the local repository!");
+
+ Verifier filteredVerifier = newVerifier(testDir);
+ filteredVerifier.setLogFileName("log3.txt");
+ filteredVerifier.setAutoclean(false);
+ filteredVerifier.addCliArgument("-pl");
+ filteredVerifier.addCliArgument("project");
+ filteredVerifier.addCliArgument("validate");
+ filteredVerifier.execute();
+ filteredVerifier.verifyErrorFreeLog();
+ filteredVerifier.verifyTextNotInLog(
+ "[WARNING] 'project' uses 'org.apache.maven.its.mng5572:plugin' as
extension which is not possible within the same reactor build. This plugin was
pulled from the local repository!");
Review Comment:
🔧 **Indentation inconsistency.** The string continuation here is indented 12
spaces, while the parallel `verifyTextInLog` call at line 58–59 uses 16 spaces.
Use 16 spaces to be consistent.
```suggestion
filteredVerifier.verifyTextNotInLog(
"[WARNING] 'project' uses
'org.apache.maven.its.mng5572:plugin' as extension which is not possible within
the same reactor build. This plugin was pulled from the local repository!");
```
--
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]
