Improve API doc for ArtifactHandler #1193

elharo · 2023-07-02T12:02:52Z

Noticed this was a unclear and out of date

hboutemy

on packaging field, i think this is the most confusing parameter that lead to misunderstandings for year: packaging field is the "probable" packaging that created the artifact (if there is any probable one, as any packaging may create a source or javadoc artifact): I'm sure this API is unused; because I don't see how it can be reasonably used

on directory, I don't know at all what the intent was

everything I wrote in the documentation (like https://maven.apache.org/ref/3.9.3/maven-core/artifact-handlers.html) is based on years of thoughts trying to understand the initial intent of the code, that was written before i was involved...

thanks for helping digging into it

hboutemy · 2023-07-03T06:28:39Z

maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java

@@ -41,7 +42,7 @@ public interface ArtifactHandler {
    String getDirectory();

    /**
-     * Get the classifier associated to the dependency type.
+     * Returns the classifier of the dependency.


while we are at clarifying, it is the default classifier: user may override the value in his pom.xml dependency (by defining a classifier)

so this is the classifier from the artifact's own pom.xml?

The ArtifactHandler is unrelated to the artifact's own pom.xml.
The ArtifactHandler is used to determine a few informations about a dependency. It is retrieved based on the dependency type. But the Dependency can define a classifier, which takes precedence over the default one provided by the artifact handler.

Looking through the code, I think this is the default classifier:only if the dependency element doesn't specify one, and tbhat's the empty string (o null?). This should only be set if the dependency element explicitly sets it.

Agreed, classifier -> default classifier

gnodet · 2023-07-03T06:26:26Z

maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java

@@ -19,7 +19,7 @@
 package org.apache.maven.artifact.handler;

 /**
- * An artifact handler defines for a dependency type, defined as Plexus role:<ul>
+ * An artifact handler provides metadata for an artifact derived from the dependency element that references the artifact:<ul>
 * <li>extension and classifier, to be able to download the file,</li>


should we talk about default extension and classifier as those can always be further specified when actually adding a dependency to those artifacts ?

Does this always provide the info from the artifact's own pom.xml?

I'm a little confused because if the classifier is used to download the file as seen in the first bullet point then it has to come from the dependency element that loads the artifact, not the artifact's own pom.xml. Maybe the first bullet point is wrong?

See below, the artifact's own pom.xml is not used at all here afaik.

on extension, there is no way to override it: then it's directly the extension
that's one reason why most people are confused by the (lack of) difference between type and extension, while there is a difference between the default classifier and the effective classifier

gnodet · 2023-07-03T07:00:34Z

maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java

@@ -41,7 +42,7 @@ public interface ArtifactHandler {
    String getDirectory();

    /**
-     * Get the classifier associated to the dependency type.
+     * Returns the classifier of the dependency.


I think this is wrong, the previous wording was better imho, as the handler does not define a dependency, but a dependency type.
So maybe something like Returns the default classifier used for dependencies of that type. or something like that ?

I'm not sure there is such a thing as a "dependency type" and I'd prefer to avoid that term completely. If it's confusing us, it will certainly confuse end users. I think there are three things in play here:

The binary artifact served from the repository, such as a jar file, a signature file, or a pom.xml file

The pom.xml for the artifact. For a jar file this is the pom.xml from which the jar was built, though I'm not sure this is meaningful for anything but the regular jar containing .class files. I'm not sure if it makes sense for javadoc or source jars or non-jars. This is the pom found in the same directory as the jar.

The pom.xml that imports the artifact using a dependency element.

This dependency can declare a different packaging and other properties of the artifact.

So does ArtifactHandler report number 3 or number 2 if both are present? Or does the answer to this question depend on which property we're looking at?

#2 is not used with ArtifactHandler afaik. The ArtifactHandler is used to compute #3 in case it is not explicitly set in the pom.

I think the concept of type leaks throughout the API. It was present in the v3 api and in the model. However, this api is somewhat flawed because the Artifact is used as both an artifact and a dependency. The v4 api tries to clear things a bit by splitting Artifact, ArtifactCoordinate, Dependency and DependencyCoordinate.

in fact, if Artifact in Maven core did not mix features from Artifact and Dependency (as Maven Resolver documented and solved 10 years later), this ArtifactHandler should have been named DependencyHandler
Because it's what that class does: define how to handle a dependency
IMHO, saying "artifact/dependency handler" in the javadoc could help understand reality

Note that the v4 api provides Type as an equivalent to v3 ArtifactHandler, directly accessible using Dependency#getType(). Maybe the api could be improved at the same time (replace artifact type with dependency type as a first step for example).

gnodet

We should make it more explicit that the artifact handler provides default values.

gnodet · 2023-07-04T12:44:04Z

maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java

@@ -19,10 +19,10 @@
 package org.apache.maven.artifact.handler;

 /**
- * An artifact handler defines for a dependency type, defined as Plexus role:<ul>
- * <li>extension and classifier, to be able to download the file,</li>
+ * An artifact handler contains metadata derived from the dependency element that references the artifact:<ul>


I don't think that's true.
The metadata is not derived from the dependency element. The set of artifact handlers is reduced and provides metadata related to the dependency type.

gnodet · 2023-07-04T12:44:44Z

maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java

- * An artifact handler defines for a dependency type, defined as Plexus role:<ul>
- * <li>extension and classifier, to be able to download the file,</li>
+ * An artifact handler contains metadata derived from the dependency element that references the artifact:<ul>
+ * <li>extension and classifier, to be able to download the file</li>


extension and classifier -> défaut extension and classifier

finally, it's more extension and default classifier

gnodet · 2023-07-04T12:45:24Z

maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java

 * <li>information on how to use the artifact: whether to add it to the classpath, or to take into account its
- * dependencies.</li>
+ * dependencies</li>


Should we keep the . and add , at the end of the previous <li>.

maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java

gnodet · 2023-07-04T12:46:17Z

maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java

@@ -41,7 +42,7 @@ public interface ArtifactHandler {
    String getDirectory();

    /**
-     * Get the classifier associated to the dependency type.
+     * Returns the classifier of the dependency.


Agreed, classifier -> default classifier

elharo

PTAL

I think the comments are no longer aplicable to the current version, but if they are we can make further changes

Rewrite API doc

c34ee5b

Noticed this was a unclear and out of date

elharo requested a review from gnodet July 2, 2023 12:02

elharo changed the title ~~Rewrite API doc~~ Rewrite API doc for ArtifactHandler Jul 2, 2023

hboutemy changed the title ~~Rewrite API doc for ArtifactHandler~~ Improve API doc for ArtifactHandler Jul 3, 2023

hboutemy previously requested changes Jul 3, 2023

View reviewed changes

gnodet requested changes Jul 3, 2023

View reviewed changes

correct text

9c82350

gnodet requested changes Jul 4, 2023

View reviewed changes

revise

c0bbc96

elharo commented Jul 14, 2023

View reviewed changes

gnodet approved these changes Aug 22, 2023

View reviewed changes

elharo merged commit 421a23a into master Aug 22, 2023
20 of 21 checks passed

elharo deleted the elharo-patch-1 branch August 22, 2023 11:52

gnodet added this to the 4.0.0-alpha-8 milestone Sep 11, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve API doc for ArtifactHandler #1193

Improve API doc for ArtifactHandler #1193

elharo commented Jul 2, 2023

hboutemy left a comment •

edited

Loading

hboutemy Jul 3, 2023

elharo Jul 3, 2023

gnodet Jul 4, 2023

elharo Jul 4, 2023

gnodet Jul 4, 2023

gnodet Jul 3, 2023

elharo Jul 3, 2023

gnodet Jul 3, 2023 •

edited

Loading

hboutemy Jul 5, 2023

gnodet Jul 3, 2023

elharo Jul 3, 2023

gnodet Jul 3, 2023

hboutemy Jul 5, 2023

gnodet Jul 5, 2023

gnodet left a comment

gnodet Jul 4, 2023

gnodet Jul 4, 2023

hboutemy Jul 5, 2023

gnodet Jul 4, 2023

gnodet Jul 4, 2023

elharo left a comment

Improve API doc for ArtifactHandler #1193

Improve API doc for ArtifactHandler #1193

Conversation

elharo commented Jul 2, 2023

hboutemy left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnodet Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnodet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

hboutemy left a comment •

edited

Loading

gnodet Jul 3, 2023 •

edited

Loading