Pankraz76 commented on code in PR #2347:
URL: https://github.com/apache/maven/pull/2347#discussion_r2094587418
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -67,6 +67,21 @@
@Named
@Singleton
public class DefaultDependencyResolver implements DependencyResolver {
+ /**
+ * Cache of information about the modules contained in a path element.
+ * This cache is created when first needed. It may be never created.
+ *
+ * <p><b>TODO:</b> This field should not be in this class, because the
cache should be global to the session.
+ * This field exists here only temporarily, until we clarified where to
store session-wide caches.</p>
+ *
+ * @see moduleCache()
+ */
+ private PathModularizationCache moduleCache;
+
+ /**
+ * Creates an initially empty resolver.
+ */
+ public DefaultDependencyResolver() {}
Review Comment:
who need this? will create very fragile object.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -97,6 +99,24 @@ public class DefaultDependencyResolverResult implements
DependencyResolverResult
*/
private final PathModularizationCache cache;
+ /**
+ * Creates an initially empty result with a temporary cache.
+ * Callers should add path elements by calls to {@link
#addDependency(Node, Dependency, Predicate, Path)}.
+ *
+ * <p><b>WARNING: this constructor may be removed in a future Maven
release.</b>
+ * The reason is because {@code DefaultDependencyResolverResult} needs a
cache, which should
+ * preferably by session-wide. But we have not yet clarified how such
caches should be managed.</p>
+ *
+ * @param request the corresponding request
+ * @param exceptions the exceptions that occurred while building the
dependency graph
+ * @param root the root node of the dependency graph
+ * @param count estimated number of dependencies
+ */
+ public DefaultDependencyResolverResult(
+ DependencyResolverRequest request, List<Exception> exceptions,
Node root, int count) {
+ this(request, new PathModularizationCache(), exceptions, root, count);
Review Comment:
yes this gives dedication and option.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -67,6 +67,21 @@
@Named
@Singleton
public class DefaultDependencyResolver implements DependencyResolver {
+ /**
+ * Cache of information about the modules contained in a path element.
+ * This cache is created when first needed. It may be never created.
+ *
+ * <p><b>TODO:</b> This field should not be in this class, because the
cache should be global to the session.
+ * This field exists here only temporarily, until we clarified where to
store session-wide caches.</p>
Review Comment:
```suggestion
* This field exists here only temporarily, until clarified where to
store session-wide caches.</p>
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -97,6 +99,24 @@ public class DefaultDependencyResolverResult implements
DependencyResolverResult
*/
private final PathModularizationCache cache;
+ /**
+ * Creates an initially empty result with a temporary cache.
+ * Callers should add path elements by calls to {@link
#addDependency(Node, Dependency, Predicate, Path)}.
+ *
+ * <p><b>WARNING: this constructor may be removed in a future Maven
release.</b>
+ * The reason is because {@code DefaultDependencyResolverResult} needs a
cache, which should
+ * preferably by session-wide. But we have not yet clarified how such
caches should be managed.</p>
+ *
+ * @param request the corresponding request
+ * @param exceptions the exceptions that occurred while building the
dependency graph
+ * @param root the root node of the dependency graph
+ * @param count estimated number of dependencies
+ */
+ public DefaultDependencyResolverResult(
+ DependencyResolverRequest request, List<Exception> exceptions,
Node root, int count) {
+ this(request, new PathModularizationCache(), exceptions, root, count);
Review Comment:
```suggestion
this(request, moduleCache(), exceptions, root, count);
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -107,14 +127,14 @@ public class DefaultDependencyResolverResult implements
DependencyResolverResult
* @param root the root node of the dependency graph
* @param count estimated number of dependencies
*/
- public DefaultDependencyResolverResult(
+ DefaultDependencyResolverResult(
DependencyResolverRequest request,
PathModularizationCache cache,
List<Exception> exceptions,
Node root,
int count) {
this.request = request;
- this.cache = cache;
+ this.cache = Objects.requireNonNull(cache);
Review Comment:
or here
```suggestion
this.cache = Objects.requireNonNull(moduleCache());
```
one of the suggestions is right. as `moduleCache()` is SSOT.
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolverResult.java:
##########
@@ -97,6 +99,24 @@ public class DefaultDependencyResolverResult implements
DependencyResolverResult
*/
private final PathModularizationCache cache;
+ /**
+ * Creates an initially empty result with a temporary cache.
+ * Callers should add path elements by calls to {@link
#addDependency(Node, Dependency, Predicate, Path)}.
+ *
+ * <p><b>WARNING: this constructor may be removed in a future Maven
release.</b>
+ * The reason is because {@code DefaultDependencyResolverResult} needs a
cache, which should
+ * preferably by session-wide. But we have not yet clarified how such
caches should be managed.</p>
Review Comment:
```suggestion
* preferably by session-wide. How to manage such caches, has not yet
been clarified.</p>
```
##########
impl/maven-impl/src/main/java/org/apache/maven/impl/DefaultDependencyResolver.java:
##########
@@ -126,7 +141,11 @@ public DependencyResolverResult collect(@Nonnull
DependencyResolverRequest reque
final CollectResult result =
session.getRepositorySystem().collectDependencies(systemSession,
collectRequest);
return new DefaultDependencyResolverResult(
- null, null, result.getExceptions(),
session.getNode(result.getRoot(), request.getVerbose()), 0);
+ null,
+ moduleCache(),
Review Comment:
as its dry, and static, inline. Ide shoud give warning on method or
constructor that value is always 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.
To unsubscribe, e-mail: issues-unsubscr...@maven.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
