gnodet commented on code in PR #12327:
URL: https://github.com/apache/maven/pull/12327#discussion_r3441833242


##########
maven-core/src/main/java/org/apache/maven/plugin/internal/PluginDependenciesResolver.java:
##########
@@ -61,12 +62,34 @@ Artifact resolve(Plugin plugin, List<RemoteRepository> 
repositories, RepositoryS
      * @param session The repository session to use for resolving the plugin 
artifacts, must not be {@code null}.
      * @return The dependency tree denoting the resolved plugin class path, 
never {@code null}.
      * @throws PluginResolutionException If any dependency could not be 
resolved.
+     * @deprecated This method should be avoided, as it needs manual 
flattening, instead to let Resolver do it. Use {@link 
#resolveAndFlatten(Plugin, Artifact, DependencyFilter, List, 
RepositorySystemSession)} instead.

Review Comment:
   Nit: `"instead to let Resolver do it"` reads awkwardly. Suggestion:
   
   ```suggestion
        * @deprecated This method should be avoided, as it requires manual 
flattening; use {@link #resolveAndFlatten(Plugin, Artifact, DependencyFilter, 
List, RepositorySystemSession)} instead to let Resolver handle it.
   ```



##########
maven-core/src/main/java/org/apache/maven/plugin/internal/PluginDependenciesResolver.java:
##########
@@ -61,12 +62,34 @@ Artifact resolve(Plugin plugin, List<RemoteRepository> 
repositories, RepositoryS
      * @param session The repository session to use for resolving the plugin 
artifacts, must not be {@code null}.
      * @return The dependency tree denoting the resolved plugin class path, 
never {@code null}.
      * @throws PluginResolutionException If any dependency could not be 
resolved.
+     * @deprecated This method should be avoided, as it needs manual 
flattening, instead to let Resolver do it. Use {@link 
#resolveAndFlatten(Plugin, Artifact, DependencyFilter, List, 
RepositorySystemSession)} instead.
      */
+    @Deprecated
     DependencyNode resolve(
             Plugin plugin,
             Artifact pluginArtifact,
             DependencyFilter dependencyFilter,
             List<RemoteRepository> repositories,
             RepositorySystemSession session)
             throws PluginResolutionException;
+
+    /**
+     * Resolves the runtime dependencies of the specified plugin.
+     *
+     * @param plugin The plugin for which to resolve the dependencies, must 
not be {@code null}.
+     * @param pluginArtifact The plugin's main artifact, may be {@code null}.
+     * @param dependencyFilter A filter to exclude artifacts from resolution 
(but not collection), may be {@code null}.
+     * @param repositories The plugin repositories to use for resolving the 
plugin artifacts, must not be {@code null}.
+     * @param session The repository session to use for resolving the plugin 
artifacts, must not be {@code null}.
+     * @return The dependency resolution result having the resolved plugin 
class path but also the tree, never {@code null}.
+     * @throws PluginResolutionException If any dependency could not be 
resolved.
+     * @since 3.10.0
+     */
+    DependencyResult resolveAndFlatten(

Review Comment:
   Question: on master (Maven 4), the equivalent method is named 
`resolvePlugin()` and `resolveCoreExtension()` already returns 
`DependencyResult` directly. The `AndFlatten` suffix is arguably more 
descriptive, but it introduces a naming divergence from Maven 4 — which is the 
opposite of the PR's stated alignment goal. Was this intentional, or would 
`resolvePlugin` / keeping `resolveCoreExtension` (with a changed return type) 
be preferred for consistency?



##########
maven-core/src/main/java/org/apache/maven/plugin/internal/DefaultPluginDependenciesResolver.java:
##########
@@ -161,20 +163,46 @@ public DependencyNode resolveCoreExtension(
             List<RemoteRepository> repositories,
             RepositorySystemSession session)
             throws PluginResolutionException {
+        return resolveInternal(plugin, null /* pluginArtifact */, 
dependencyFilter, repositories, session)
+                .getRoot();
+    }
+
+    /**
+     * @since 3.10.0
+     */
+    public DependencyResult resolveCoreExtensionAndFlatten(

Review Comment:
   This method is `public` but not declared on the `PluginDependenciesResolver` 
interface. `BootstrapCoreExtensionManager` calls it, so it depends on the 
concrete type. The old `resolveCoreExtension` had the same pattern, but since 
the interface is already being touched to add `resolveAndFlatten`, would it 
make sense to also promote this to the interface?



-- 
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]

Reply via email to