You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "desruisseaux (via GitHub)" <gi...@apache.org> on 2024/08/10 12:12:42 UTC

[PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

desruisseaux opened a new pull request, #1640:
URL: https://github.com/apache/maven/pull/1640

   The pull request contains two changes:
   
   * Documentation on `Artifact`, `ArtifactCoordinate`, `Dependency` and `DependencyCoordinate`.
   * Renaming of `ArtifactCoordinate.getVersion()` as `getVersionConstraint()`.
   
   This pull request is located after #1621 for avoiding merge conflict. It would be easier to merge (if accepted) #1621 before to merge this one.


-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #1640:
URL: https://github.com/apache/maven/pull/1640#discussion_r1714920456


##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.
+ * {@code Artifact} instances are created when <dfn>resolving</dfn> {@link Artifact} instances.
+ * Resolving is the process that selects a {@linkplain #getVersion() particular version}
+ * and downloads the artifact in the local repository.
+ * The download may be deferred to the first time that the file is needed.
  *
  * @since 4.0.0
  */
 @Experimental
 @Immutable
 public interface Artifact {
-
     /**
-     * Returns a unique identifier for this artifact.
+     * {@return a unique identifier for this artifact}.
      * The identifier is composed of groupId, artifactId, extension, classifier, and version.
      *
-     * @return the unique identifier
+     * @see ArtifactCoordinate#getId()
      */
     @Nonnull
     default String key() {
+        String c = getClassifier();
         return getGroupId()
                 + ':'
                 + getArtifactId()
                 + ':'
                 + getExtension()
-                + (getClassifier().isEmpty() ? "" : ":" + getClassifier())
+                + (c.isEmpty() ? "" : ":" + c)
                 + ':'
                 + getVersion();
     }
 
     /**
-     * The groupId of the artifact.
+     * {@return the group identifier of the artifact}.
      *
-     * @return the groupId
+     * @see ArtifactCoordinate#getGroupId()
      */
     @Nonnull
     String getGroupId();
 
     /**
-     * The artifactId of the artifact.
+     * {@return the identifier of the artifact}.
      *
-     * @return the artifactId
+     * @see ArtifactCoordinate#getArtifactId()
      */
     @Nonnull
     String getArtifactId();
 
     /**
-     * The version of the artifact.
+     * {@return the version of the artifact}. Contrarily to {@link ArtifactCoordinate},
+     * each {@code Artifact} is associated to a specific version instead of a range of versions.

Review Comment:
   Then the `Version` interface should said something about those meta-versions. I will put a first attempt, but it should be completed with an explanation of `LATEST`, `RELEASE` and `SNAPSHOT` semantic.



-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1640:
URL: https://github.com/apache/maven/pull/1640#discussion_r1713056325


##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.

Review Comment:
   We can't use `resolved` here imho.  An `Artifact` may not be resolved / downloaded.
   For example, the result of the `DependencyResolver#collect()` method will return a graph of `Node`, each one containing a `Dependency` (which extends `Artifact`), without having downloaded the related files.



##########
api/maven-api-core/src/main/java/org/apache/maven/api/Dependency.java:
##########
@@ -23,30 +23,45 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
+ * Result of resolving a {@code DependencyCoordinate}.
+ * Resolving is the process that clarifies the obligation (optional or mandatory status),
+ * selects a particular version and downloads the artifact in the local repository.
  *
  * @since 4.0.0
  */
 @Experimental
 @Immutable
 public interface Dependency extends Artifact {
-
     /**
-     * The dependency type.
+     * {@return the type of the dependency}. A dependency can be a <abbr>JAR</abbr> file,
+     * a modular-<abbr>JAR</abbr> if it is intended to be placed on the module-path,
+     * a <abbr>JAR</abbr> containing test classes, <i>etc.</i>
      *
-     * @return the dependency type, never {@code null}
+     * @see DependencyCoordinate#getType()
      */
     @Nonnull
     Type getType();
 
+    /**
+     * {@return the time at which the dependency will be used}.
+     * If may be, for example, at compile time only, at run time or at test time.
+     *
+     * @see DependencyCoordinate#getScope()
+     */
     @Nonnull
     DependencyScope getScope();
 
+    /**
+     * Returns whether the dependency is optional or mandatory.
+     * Contrarily to {@link DependencyCoordinate}, the obligation of a dependency cannot be unspecified.

Review Comment:
   It's not specified at all, it's computed.  The collection phase will compute the scope and optionality of each dependency.



##########
api/maven-api-core/src/main/java/org/apache/maven/api/ArtifactCoordinate.java:
##########
@@ -23,64 +23,68 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * The {@code Coordinate} object is used to point to an {@link Artifact}
- * but the version may be specified as a range instead of an exact version.
+ * Point to an {@link Artifact} but with the version specified as a range instead of an exact version.
+ * Each {@code ArtifactCoordinate} instance is basically a pointer to a file in the Maven repository,
+ * except that the version may not be defined precisely.

Review Comment:
   Not sure I like the wording, since the actual pointer is the `Artifact`. `ArtifactCoordinate` is used to create those pointers by resolving the version.
   
   I think relocations do not come into play for artifacts, but only for dependencies, at least that's what the [resolver ArtifactResolver's doc seems to indicate](https://github.com/apache/maven-resolver/blob/97dfd1c2b9deb15734d5e401807e55cd0498332a/maven-resolver-impl/src/main/java/org/eclipse/aether/impl/ArtifactResolver.java#L41-L43)



##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.
+ * {@code Artifact} instances are created when <dfn>resolving</dfn> {@link Artifact} instances.
+ * Resolving is the process that selects a {@linkplain #getVersion() particular version}
+ * and downloads the artifact in the local repository.
+ * The download may be deferred to the first time that the file is needed.
  *
  * @since 4.0.0
  */
 @Experimental
 @Immutable
 public interface Artifact {
-
     /**
-     * Returns a unique identifier for this artifact.
+     * {@return a unique identifier for this artifact}.
      * The identifier is composed of groupId, artifactId, extension, classifier, and version.
      *
-     * @return the unique identifier
+     * @see ArtifactCoordinate#getId()
      */
     @Nonnull
     default String key() {
+        String c = getClassifier();
         return getGroupId()
                 + ':'
                 + getArtifactId()
                 + ':'
                 + getExtension()
-                + (getClassifier().isEmpty() ? "" : ":" + getClassifier())
+                + (c.isEmpty() ? "" : ":" + c)
                 + ':'
                 + getVersion();
     }
 
     /**
-     * The groupId of the artifact.
+     * {@return the group identifier of the artifact}.
      *
-     * @return the groupId
+     * @see ArtifactCoordinate#getGroupId()
      */
     @Nonnull
     String getGroupId();
 
     /**
-     * The artifactId of the artifact.
+     * {@return the identifier of the artifact}.
      *
-     * @return the artifactId
+     * @see ArtifactCoordinate#getArtifactId()
      */
     @Nonnull
     String getArtifactId();
 
     /**
-     * The version of the artifact.
+     * {@return the version of the artifact}. Contrarily to {@link ArtifactCoordinate},
+     * each {@code Artifact} is associated to a specific version instead of a range of versions.

Review Comment:
   range or meta version (`LATEST`, `RELEASE`, `SNAPSHOT`)



##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.
+ * {@code Artifact} instances are created when <dfn>resolving</dfn> {@link Artifact} instances.
+ * Resolving is the process that selects a {@linkplain #getVersion() particular version}
+ * and downloads the artifact in the local repository.
+ * The download may be deferred to the first time that the file is needed.
  *
  * @since 4.0.0
  */
 @Experimental
 @Immutable
 public interface Artifact {
-
     /**
-     * Returns a unique identifier for this artifact.
+     * {@return a unique identifier for this artifact}.
      * The identifier is composed of groupId, artifactId, extension, classifier, and version.
      *
-     * @return the unique identifier
+     * @see ArtifactCoordinate#getId()
      */
     @Nonnull
     default String key() {
+        String c = getClassifier();
         return getGroupId()
                 + ':'
                 + getArtifactId()
                 + ':'
                 + getExtension()
-                + (getClassifier().isEmpty() ? "" : ":" + getClassifier())
+                + (c.isEmpty() ? "" : ":" + c)
                 + ':'
                 + getVersion();
     }
 
     /**
-     * The groupId of the artifact.
+     * {@return the group identifier of the artifact}.
      *
-     * @return the groupId
+     * @see ArtifactCoordinate#getGroupId()
      */
     @Nonnull
     String getGroupId();
 
     /**
-     * The artifactId of the artifact.
+     * {@return the identifier of the artifact}.
      *
-     * @return the artifactId
+     * @see ArtifactCoordinate#getArtifactId()
      */
     @Nonnull
     String getArtifactId();
 
     /**
-     * The version of the artifact.
+     * {@return the version of the artifact}. Contrarily to {@link ArtifactCoordinate},
+     * each {@code Artifact} is associated to a specific version instead of a range of versions.
      *
-     * @return the version
+     * @see ArtifactCoordinate#getVersionConstraint()
      */
     @Nonnull
     Version getVersion();
 
     /**
-     * The base version of the artifact.
-     *
-     * @return the version
+     * {@return the base version of the artifact}.
+     * TODO: this javadoc is not helpful.

Review Comment:
   Agreed !
   So the base version is the resolved range version or the meta version (i.e. it can be `4.0.0-SNAPSHOT`) while the `getVersion()` should return `4.0.0-20240809.104932-36`).
   I'm actually not quite sure if the original `VersionConstraint` was a range... @cstamas  ?



##########
api/maven-api-core/src/main/java/org/apache/maven/api/ArtifactCoordinate.java:
##########
@@ -23,64 +23,68 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * The {@code Coordinate} object is used to point to an {@link Artifact}
- * but the version may be specified as a range instead of an exact version.
+ * Point to an {@link Artifact} but with the version specified as a range instead of an exact version.
+ * Each {@code ArtifactCoordinate} instance is basically a pointer to a file in the Maven repository,
+ * except that the version may not be defined precisely.
  *
  * @since 4.0.0
  */
 @Experimental
 @Immutable
 public interface ArtifactCoordinate {
-
     /**
-     * The groupId of the artifact.
-     *
-     * @return the groupId
+     * {@return the group identifier of the artifact}.
      */
     @Nonnull
     String getGroupId();
 
     /**
-     * The artifactId of the artifact.
-     *
-     * @return the artifactId
+     * {@return the identifier of the artifact}.
      */
     @Nonnull
     String getArtifactId();
 
     /**
-     * The classifier of the artifact.
+     * Returns the classifier of the artifact.
      *
      * @return the classifier or an empty string if none, never {@code null}
      */
     @Nonnull
     String getClassifier();
 
     /**
-     * The version of the artifact.
-     *
-     * @return the version
+     * {@return the range of versions of the artifact}.

Review Comment:
   Specific version, range, or meta version.



##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.
+ * {@code Artifact} instances are created when <dfn>resolving</dfn> {@link Artifact} instances.

Review Comment:
   We do resolve `ArtifactCoordinate`s



##########
api/maven-api-core/src/main/java/org/apache/maven/api/Dependency.java:
##########
@@ -23,30 +23,45 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
+ * Result of resolving a {@code DependencyCoordinate}.
+ * Resolving is the process that clarifies the obligation (optional or mandatory status),
+ * selects a particular version and downloads the artifact in the local repository.

Review Comment:
   It's a bit more complicated as explained earlier.  `Dependency` is the output of the `collection` process, which builds the graph of dependencies. It then needs to be `flattened` and `resolved`.  The version selection is done for each dependency during the collection phase, the flatten phase will keep only a single version per GA (groupId+artifactId), the resolution will actually download the dependencies (or artifacts) that have been computed.



##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.
+ * {@code Artifact} instances are created when <dfn>resolving</dfn> {@link Artifact} instances.
+ * Resolving is the process that selects a {@linkplain #getVersion() particular version}
+ * and downloads the artifact in the local repository.

Review Comment:
   So things are a bit blurry:
    * artifact resolution is:  resolve version (see `VersionResolver#resolveVersion()`), and then resolve (i.e. download) the file
    * dependency resolution is: collect dependencies, flatten the graph, resolve (i.e. download) the file
   
   During dependency collection, versions are resolved (@cstamas right?). The flattening phase removes branches of the graph so that one artifact per GA is present. The resolution phase will then download the result artifacts.
   
   We don't have any way to download an artifact for which the version has been resolved though (that may be a missing bit).  So if we introduce `ResolvedArtifact` and `ResolvedDependency`, it may help a bit:
     * version resolution: ArtifactCoordinate -> Artifact
     * artifact resolution: Artifact -> ResolvedArtifact
     * dependency collection -> Node + Dependency
     * dependency resolution: list of Dependency -> list of ResolvedDependency



##########
api/maven-api-core/src/main/java/org/apache/maven/api/DependencyScope.java:
##########
@@ -28,9 +28,9 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * Dependency scope.
- * This represents at which time the dependency will be used, for example, at compile time only,
- * at run time or at test time.  For a given dependency, the scope is directly derived from the
+ * Represents at which time the dependency will be used.
+ * If may be, for example, at compile time only, at run time or at test time.

Review Comment:
   `Indicates when the dependency will be used. For example, it may be at compile time only, at runtime, or at test time.`



-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #1640:
URL: https://github.com/apache/maven/pull/1640#discussion_r1714881957


##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.

Review Comment:
   I thought that "resolved" means that all properties were fixed to their final values, especially the version and obligation. Before resolution, an `ArtifactCoordinate` refers to an ensemble of files. After resolution, an `Artifact` refers to a single file. I was seeing the download as a separated concern. I have no problem in including download in the definition of "resolved", but maybe we should write this definition somewhere, e.g. in the package-info file.



-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #1640:
URL: https://github.com/apache/maven/pull/1640#discussion_r1714911250


##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.
+ * {@code Artifact} instances are created when <dfn>resolving</dfn> {@link Artifact} instances.
+ * Resolving is the process that selects a {@linkplain #getVersion() particular version}
+ * and downloads the artifact in the local repository.

Review Comment:
   Added a "definitions of terms" section in `package-info.java` with the above explanation about artifact resolution and dependency resolution. Since those definitions include the download part, they answer my [previous comment](https://github.com/apache/maven/pull/1640#discussion_r1714881957).



-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #1640:
URL: https://github.com/apache/maven/pull/1640#discussion_r1715015611


##########
api/maven-api-core/src/main/java/org/apache/maven/api/ArtifactCoordinate.java:
##########
@@ -23,64 +23,68 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * The {@code Coordinate} object is used to point to an {@link Artifact}
- * but the version may be specified as a range instead of an exact version.
+ * Point to an {@link Artifact} but with the version specified as a range instead of an exact version.
+ * Each {@code ArtifactCoordinate} instance is basically a pointer to a file in the Maven repository,
+ * except that the version may not be defined precisely.

Review Comment:
   Replaced by "Identification of an ensemble of {@link Artifact}s in a range of versions."



-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #1640:
URL: https://github.com/apache/maven/pull/1640#discussion_r1719600158


##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.
+ * {@code Artifact} instances are created when <dfn>resolving</dfn> {@link Artifact} instances.
+ * Resolving is the process that selects a {@linkplain #getVersion() particular version}
+ * and downloads the artifact in the local repository.
+ * The download may be deferred to the first time that the file is needed.
  *
  * @since 4.0.0
  */
 @Experimental
 @Immutable
 public interface Artifact {
-
     /**
-     * Returns a unique identifier for this artifact.
+     * {@return a unique identifier for this artifact}.
      * The identifier is composed of groupId, artifactId, extension, classifier, and version.
      *
-     * @return the unique identifier
+     * @see ArtifactCoordinate#getId()
      */
     @Nonnull
     default String key() {
+        String c = getClassifier();
         return getGroupId()
                 + ':'
                 + getArtifactId()
                 + ':'
                 + getExtension()
-                + (getClassifier().isEmpty() ? "" : ":" + getClassifier())
+                + (c.isEmpty() ? "" : ":" + c)
                 + ':'
                 + getVersion();
     }
 
     /**
-     * The groupId of the artifact.
+     * {@return the group identifier of the artifact}.
      *
-     * @return the groupId
+     * @see ArtifactCoordinate#getGroupId()
      */
     @Nonnull
     String getGroupId();
 
     /**
-     * The artifactId of the artifact.
+     * {@return the identifier of the artifact}.
      *
-     * @return the artifactId
+     * @see ArtifactCoordinate#getArtifactId()
      */
     @Nonnull
     String getArtifactId();
 
     /**
-     * The version of the artifact.
+     * {@return the version of the artifact}. Contrarily to {@link ArtifactCoordinate},
+     * each {@code Artifact} is associated to a specific version instead of a range of versions.

Review Comment:
   I forgot that `LATEST` and `RELEASE` are actually deprecated.  So we should not document them I think.



-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet closed pull request #1640: Renaming of `ArtifactCoordinate.getVersion()` + documentation
URL: https://github.com/apache/maven/pull/1640


-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on PR #1640:
URL: https://github.com/apache/maven/pull/1640#issuecomment-2294933659

   Thanks, it looks good to me. We may need to revisit #1625 after this pull request is merged.


-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #1640:
URL: https://github.com/apache/maven/pull/1640#discussion_r1714939751


##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.
+ * {@code Artifact} instances are created when <dfn>resolving</dfn> {@link Artifact} instances.
+ * Resolving is the process that selects a {@linkplain #getVersion() particular version}
+ * and downloads the artifact in the local repository.
+ * The download may be deferred to the first time that the file is needed.
  *
  * @since 4.0.0
  */
 @Experimental
 @Immutable
 public interface Artifact {
-
     /**
-     * Returns a unique identifier for this artifact.
+     * {@return a unique identifier for this artifact}.
      * The identifier is composed of groupId, artifactId, extension, classifier, and version.
      *
-     * @return the unique identifier
+     * @see ArtifactCoordinate#getId()
      */
     @Nonnull
     default String key() {
+        String c = getClassifier();
         return getGroupId()
                 + ':'
                 + getArtifactId()
                 + ':'
                 + getExtension()
-                + (getClassifier().isEmpty() ? "" : ":" + getClassifier())
+                + (c.isEmpty() ? "" : ":" + c)
                 + ':'
                 + getVersion();
     }
 
     /**
-     * The groupId of the artifact.
+     * {@return the group identifier of the artifact}.
      *
-     * @return the groupId
+     * @see ArtifactCoordinate#getGroupId()
      */
     @Nonnull
     String getGroupId();
 
     /**
-     * The artifactId of the artifact.
+     * {@return the identifier of the artifact}.
      *
-     * @return the artifactId
+     * @see ArtifactCoordinate#getArtifactId()
      */
     @Nonnull
     String getArtifactId();
 
     /**
-     * The version of the artifact.
+     * {@return the version of the artifact}. Contrarily to {@link ArtifactCoordinate},
+     * each {@code Artifact} is associated to a specific version instead of a range of versions.
      *
-     * @return the version
+     * @see ArtifactCoordinate#getVersionConstraint()
      */
     @Nonnull
     Version getVersion();
 
     /**
-     * The base version of the artifact.
-     *
-     * @return the version
+     * {@return the base version of the artifact}.
+     * TODO: this javadoc is not helpful.

Review Comment:
   So does it means that the discussion about meta-versions in [above comment](https://github.com/apache/maven/pull/1640#discussion_r1713059218) actually applies here?



-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1640:
URL: https://github.com/apache/maven/pull/1640#issuecomment-2283554760

   @desruisseaux One possibility would be to split this PR.  Keep this one for the doc, and create another one for the getVersionConstraint() method if that allows you to go further.


-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on PR #1640:
URL: https://github.com/apache/maven/pull/1640#issuecomment-2285909084

   Added a commit trying to resolve the comments to the best of my understanding. Did à `git push --force` for resolving conflicts.


-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "desruisseaux (via GitHub)" <gi...@apache.org>.
desruisseaux commented on code in PR #1640:
URL: https://github.com/apache/maven/pull/1640#discussion_r1719628286


##########
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java:
##########
@@ -23,76 +23,83 @@
 import org.apache.maven.api.annotations.Nonnull;
 
 /**
- * An artifact points to a resource such as a jar file or war application.
+ * Pointer to a resolved resource such as a <abbr>JAR</abbr> file or <abbr>WAE</abbr> application.
+ * {@code Artifact} instances are created when <dfn>resolving</dfn> {@link Artifact} instances.
+ * Resolving is the process that selects a {@linkplain #getVersion() particular version}
+ * and downloads the artifact in the local repository.
+ * The download may be deferred to the first time that the file is needed.
  *
  * @since 4.0.0
  */
 @Experimental
 @Immutable
 public interface Artifact {
-
     /**
-     * Returns a unique identifier for this artifact.
+     * {@return a unique identifier for this artifact}.
      * The identifier is composed of groupId, artifactId, extension, classifier, and version.
      *
-     * @return the unique identifier
+     * @see ArtifactCoordinate#getId()
      */
     @Nonnull
     default String key() {
+        String c = getClassifier();
         return getGroupId()
                 + ':'
                 + getArtifactId()
                 + ':'
                 + getExtension()
-                + (getClassifier().isEmpty() ? "" : ":" + getClassifier())
+                + (c.isEmpty() ? "" : ":" + c)
                 + ':'
                 + getVersion();
     }
 
     /**
-     * The groupId of the artifact.
+     * {@return the group identifier of the artifact}.
      *
-     * @return the groupId
+     * @see ArtifactCoordinate#getGroupId()
      */
     @Nonnull
     String getGroupId();
 
     /**
-     * The artifactId of the artifact.
+     * {@return the identifier of the artifact}.
      *
-     * @return the artifactId
+     * @see ArtifactCoordinate#getArtifactId()
      */
     @Nonnull
     String getArtifactId();
 
     /**
-     * The version of the artifact.
+     * {@return the version of the artifact}. Contrarily to {@link ArtifactCoordinate},
+     * each {@code Artifact} is associated to a specific version instead of a range of versions.

Review Comment:
   Done the removal of `LATEST` and `RELEASE` from the documentation.



-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


Re: [PR] Renaming of `ArtifactCoordinate.getVersion()` + documentation [maven]

Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on PR #1640:
URL: https://github.com/apache/maven/pull/1640#issuecomment-2294924267

   @desruisseaux I've added follow-on commits at https://github.com/gnodet/maven/tree/refactor/getVersionConstraint, containing a proposal to add `ResolvedArtifact`, `ResolvedDependency` and `ProvidedArtifact` interfaces and another one with more javadoc.


-- 
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-unsubscribe@maven.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org