Renaming of ArtifactCoordinate.getVersion() + documentation
#1640
Conversation
| @@ -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. | |||
There was a problem hiding this 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.
There was a problem hiding this 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.
api/maven-api-core/src/main/java/org/apache/maven/api/Artifact.java
Outdated
Show resolved
Hide resolved
| * 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. |
There was a problem hiding this 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
There was a problem hiding this 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.
| */ | ||
| @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. |
There was a problem hiding this comment.
range or meta version (LATEST, RELEASE, SNAPSHOT)
There was a problem hiding this 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.
There was a problem hiding this comment.
I forgot that LATEST and RELEASE are actually deprecated. So we should not document them I think.
There was a problem hiding this comment.
Done the removal of LATEST and RELEASE from the documentation.
| * | ||
| * @return the version | ||
| * {@return the base version of the artifact}. | ||
| * TODO: this javadoc is not helpful. |
There was a problem hiding this 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 ?
There was a problem hiding this comment.
So does it means that the discussion about meta-versions in above comment actually applies here?
api/maven-api-core/src/main/java/org/apache/maven/api/ArtifactCoordinate.java
Show resolved
Hide resolved
api/maven-api-core/src/main/java/org/apache/maven/api/ArtifactCoordinate.java
Outdated
Show resolved
Hide resolved
api/maven-api-core/src/main/java/org/apache/maven/api/Dependency.java
Outdated
Show resolved
Hide resolved
api/maven-api-core/src/main/java/org/apache/maven/api/Dependency.java
Outdated
Show resolved
Hide resolved
api/maven-api-core/src/main/java/org/apache/maven/api/DependencyScope.java
Outdated
Show resolved
Hide resolved
|
@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. |
…endencyCoordinate. Rename ArtifactCoordinate.getVersion() as getVersionConstraint().
desruisseaux
force-pushed
the
refactor/getVersionConstraint
branch
from
748d241
to
55e7a75
Compare
|
Added a commit trying to resolve the comments to the best of my understanding. Did à |
…ecated. Opportunistic addition of some missing `@Override` annotations.
|
@desruisseaux I've added follow-on commits at https://github.com/gnodet/maven/tree/refactor/getVersionConstraint, containing a proposal to add |
|
Thanks, it looks good to me. We may need to revisit #1625 after this pull request is merged. |
The pull request contains two changes:
Artifact,ArtifactCoordinate,DependencyandDependencyCoordinate.ArtifactCoordinate.getVersion()asgetVersionConstraint().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.