Refactoring: MavenDependencyMediation with AnnotatedClassPath #2013
Conversation
suztomo
changed the title
| import com.google.cloud.tools.opensource.dependencies.DependencyGraph; | ||
|
|
||
| /** | ||
| * Algorithm to select artifacts when there are multiple versions for the same groupId and |
There was a problem hiding this comment.
What about classifiers and types?
There was a problem hiding this comment.
Clarified in the next paragraph. This doesn't read them.
| boolean selects(Artifact artifact) { | ||
| private boolean selects(Artifact artifact) { | ||
| return artifacts.contains(artifact); | ||
| } | ||
|
|
||
| void put(DependencyPath dependencyPath) { | ||
| private void put(DependencyPath dependencyPath) { | ||
| Artifact artifact = dependencyPath.getLeaf(); |
There was a problem hiding this comment.
The method put and selects assumed how they are called when traversing dependency graph. Because the call order was not applicable to Gradle, I didn't choose them as DependencyMediation's interface method.
| * @param dependencyGraph dependency graph that may have duplicate artifacts that have the same | ||
| * groupId and artifactId combination | ||
| */ | ||
| AnnotatedClassPath mediate(DependencyGraph dependencyGraph); |
There was a problem hiding this comment.
This interface should work both Maven and Gradle.
| new ClassPathResult(LinkedListMultimap.create(), ImmutableList.of()), | ||
| new ClassPathResult(new AnnotatedClassPath(), ImmutableList.of()), |
There was a problem hiding this comment.
Removing obscure LinkedListMultimap in favor of AnnotatedClassPath, which deliver meaning in its name.
| for (DependencyPath dependencyPath : dependencyPaths) { | ||
| Artifact artifact = dependencyPath.getLeaf(); | ||
| mediation.put(dependencyPath); | ||
| if (mediation.selects(artifact)) { |
There was a problem hiding this comment.
This has moved to MavenDependencyMediation. The logic of calling selects and put are too Maven specific; Gradle needs to select artifacts after visiting all nodes in the graph.
| private final ImmutableListMultimap<ClassPathEntry, DependencyPath> dependencyPaths; | ||
| private final AnnotatedClassPath annotatedClassPath; |
There was a problem hiding this comment.
The meaning of the variable with now clearer with more concrete type.
| import com.google.cloud.tools.opensource.dependencies.DependencyGraph; | ||
|
|
||
| /** | ||
| * Algorithm to select artifacts when there are multiple versions for the same groupId and |
There was a problem hiding this comment.
Clarified in the next paragraph. This doesn't read them.
| @@ -21,6 +21,8 @@ | |||
| /** | |||
| * Algorithm to select artifacts when there are multiple versions for the same groupId and | |||
| * artifactId in a dependency graph. | |||
| * | |||
| * <p>The algorithm does not take classifier or extension into account.</p> | |||
There was a problem hiding this comment.
Memo: Maven coordinates has <groupId>:<artifactId>[:<extension>[:<classifier>]]:<version> format.
|
@elharo PTAL |
| return ImmutableList.copyOf(classPathEntryToDependencyPaths.keySet()); | ||
| } | ||
|
|
||
| ImmutableList<DependencyPath> dependencyPathOf(ClassPathEntry classPathEntry) { |
There was a problem hiding this comment.
Not sure, but feels like this maybe should be plural. I.e. I misread this when I first read it. Maybe pathsTo?
There was a problem hiding this comment.
Updated to pathsTo. Thank you.
| * Returns an {@link AnnotatedClassPath} that has class path entry and dependency paths | ||
| * relationship represented in {@code classPathEntryToDependencyPaths}. | ||
| */ | ||
| public static AnnotatedClassPath fromMultimap( |
There was a problem hiding this comment.
This appears to only be used by tests. Maybe non-public and/or visible for testing?
There was a problem hiding this comment.
Added @VisibleForTesting.
DependencyMediationinterface before implementing GradleDependencyMediation implementation for Dashboard and Linkage Checker to take option to resolve dependency graphs with Gradle's way #2007.LinkedListMultimap was obscure to deliver the meaning. The
AnnotatedClassPathdelivers more concrete meaning in code.This is a refactoring bofore implementing
GradleDependencyMediation. This PR has no behavior change.