[GitHub] [maven] gnodet commented on a diff in pull request #1193: Improve API doc for ArtifactHandler
gnodet commented on code in PR #1193:
URL: https://github.com/apache/maven/pull/1193#discussion_r1252673778
##
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.
Review Comment:
Note that the v4 api provides
[`Type`](https://github.com/apache/maven/blob/master/api/maven-api-core/src/main/java/org/apache/maven/api/Type.java)
as an equivalent to v3 `ArtifactHandler`, directly accessible using
[`Dependency#getType()`](https://github.com/apache/maven/blob/master/api/maven-api-core/src/main/java/org/apache/maven/api/Dependency.java#L25-L31).
Maybe the api could be improved at the same time (replace _artifact type_
with _dependency type_ as a first step for example).
--
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
[GitHub] [maven] gnodet commented on a diff in pull request #1193: Improve API doc for ArtifactHandler
gnodet commented on code in PR #1193:
URL: https://github.com/apache/maven/pull/1193#discussion_r1251983850
##
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:
- * extension and classifier, to be able to download the file,
+ * An artifact handler contains metadata derived from the dependency element
that references the artifact:
+ * extension and classifier, to be able to download the file
* information on how to use the artifact: whether to add it to the
classpath, or to take into account its
- * dependencies.
+ * dependencies
Review Comment:
Should we keep the `.` and add `,` at the end of the previous ``.
##
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:
- * extension and classifier, to be able to download the file,
+ * An artifact handler contains metadata derived from the dependency element
that references the artifact:
Review Comment:
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`.
##
maven-artifact/src/main/java/org/apache/maven/artifact/handler/ArtifactHandler.java:
##
@@ -32,7 +32,8 @@ public interface ArtifactHandler {
String ROLE = ArtifactHandler.class.getName();
/**
- * Get the file extension associated to the file represented by the
dependency type.
+ * Returns the file name extension of the artifact;
Review Comment:
`the file name extension` -> `the default file name extension`
##
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.
Review Comment:
Agreed, `classifier` -> `default classifier`
##
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:
- * extension and classifier, to be able to download the file,
+ * An artifact handler contains metadata derived from the dependency element
that references the artifact:
+ * extension and classifier, to be able to download the file
Review Comment:
`extension and classifier` -> `défaut extension and classifier`
--
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
[GitHub] [maven] gnodet commented on a diff in pull request #1193: Improve API doc for ArtifactHandler
gnodet commented on code in PR #1193:
URL: https://github.com/apache/maven/pull/1193#discussion_r1251554945
##
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.
Review Comment:
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_](https://maven.apache.org/ref/4-LATEST/apidocs/org/apache/maven/api/model/Dependency.html#getType()).
But the
[`Dependency`](https://maven.apache.org/ref/4-LATEST/apidocs/org/apache/maven/api/model/Dependency.html)
can define a classifier, which takes precedence over the default one provided
by the artifact handler.
--
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
[GitHub] [maven] gnodet commented on a diff in pull request #1193: Improve API doc for ArtifactHandler
gnodet commented on code in PR #1193: URL: https://github.com/apache/maven/pull/1193#discussion_r1250964574 ## 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: + * An artifact handler provides metadata for an artifact derived from the dependency element that references the artifact: * extension and classifier, to be able to download the file, Review Comment: See below, the artifact's own pom.xml is not used at all here afaik. -- 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
[GitHub] [maven] gnodet commented on a diff in pull request #1193: Improve API doc for ArtifactHandler
gnodet commented on code in PR #1193: URL: https://github.com/apache/maven/pull/1193#discussion_r1250964574 ## 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: + * An artifact handler provides metadata for an artifact derived from the dependency element that references the artifact: * extension and classifier, to be able to download the file, Review Comment: See below, the artefacts own pom.xml is not used at all here afaik. -- 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
[GitHub] [maven] gnodet commented on a diff in pull request #1193: Improve API doc for ArtifactHandler
gnodet commented on code in PR #1193:
URL: https://github.com/apache/maven/pull/1193#discussion_r1250963714
##
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.
Review Comment:
#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](https://github.com/apache/maven/blob/master/maven-artifact/src/main/java/org/apache/maven/artifact/Artifact.java#L76)
and in the
[model](https://github.com/apache/maven/blob/master/api/maven-api-model/src/main/mdo/maven.mdo#L1178).
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`](https://github.com/apache/maven/blob/master/api/maven-api-core/src/main/java/org/apache/maven/api/).
--
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
[GitHub] [maven] gnodet commented on a diff in pull request #1193: Improve API doc for ArtifactHandler
gnodet commented on code in PR #1193:
URL: https://github.com/apache/maven/pull/1193#discussion_r1250355218
##
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.
Review Comment:
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 ?
##
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:
+ * An artifact handler provides metadata for an artifact derived from the
dependency element that references the artifact:
* extension and classifier, to be able to download the file,
Review Comment:
should we talk about _default_ extension and classifier as those can always
be further specified when actually adding a dependency to those artifacts ?
--
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
