You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by "gnodet (via GitHub)" <gi...@apache.org> on 2026/09/03 21:25:56 UTC
[PR] Switch core API value types to noun-style accessors [maven]
gnodet opened a new pull request, #13036:
URL: https://github.com/apache/maven/pull/13036
Add noun-style accessors (record-accessor pattern) to core API interfaces for immutable value types, consistent with modern JDK conventions and existing Maven 4 API types (`XmlNode`, `PathType`, `Lifecycle.Phase`, etc.).
For each method, the noun-style accessor is now the primary abstract method. The existing `getX()` method is kept as a default delegating to it, annotated `@Deprecated(since = "4.1.0", forRemoval = true)`.
**Interfaces migrated:** `Artifact`, `ArtifactCoordinates`, `Dependency`, `DependencyCoordinates`, `DownloadedArtifact`, `Exclusion`, `Project`, `VersionConstraint`, `VersionRange`.
24 files changed, 821 insertions, 212 deletions.
Fixes #13035
--
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] Switch core API value types to noun-style accessors [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet merged PR #13036:
URL: https://github.com/apache/maven/pull/13036
--
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] Switch core API value types to noun-style accessors [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13036:
URL: https://github.com/apache/maven/pull/13036#discussion_r4024263121
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Node.java:
##########
@@ -40,37 +40,98 @@
@Provider
public interface Node {
+ /**
+ * Returns the artifact for this node.
+ *
+ * @return artifact for this node, or {@code null} if none
+ */
+ @Nullable
+ Artifact artifact();
+
+ /**
+ * Returns the dependency for this node.
+ *
+ * @return dependency for this node, or {@code null} if none
+ */
+ @Nullable
+ Dependency dependency();
+
+ /**
+ * Gets the child nodes of this node.
+ *
+ * @return the child nodes of this node, never {@code null}
+ */
+ @Nonnull
+ List<Node> children();
+
+ /**
+ * Returns the remote repositories of this node.
+ *
+ * @return repositories of this node, never {@code null}
+ */
+ @Nonnull
+ List<RemoteRepository> remoteRepositories();
+
+ /**
+ * The repository where this artifact has been downloaded from.
+ *
+ * @return the repository, never {@code null}
+ */
+ @Nonnull
+ Optional<RemoteRepository> repository();
Review Comment:
Addressed in a separate PR #13153, which implements `Node.getRepository()` via `LocalRepositoryManager.find()` (no network I/O) and fixes the Javadoc. The implementation in this PR keeps the existing `UnsupportedOperationException` to keep the concerns separate.
--
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] Switch core API value types to noun-style accessors [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13036:
URL: https://github.com/apache/maven/pull/13036#discussion_r4047390450
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Node.java:
##########
@@ -40,39 +40,98 @@
@Provider
public interface Node {
+ /**
+ * Returns the artifact for this node.
+ *
+ * @return artifact for this node, or {@code null} if none
+ */
+ @Nullable
+ Artifact artifact();
+
+ /**
+ * Returns the dependency for this node.
+ *
+ * @return dependency for this node, or {@code null} if none
+ */
+ @Nullable
+ Dependency dependency();
+
+ /**
+ * Gets the child nodes of this node.
+ *
+ * @return the child nodes of this node, never {@code null}
+ */
+ @Nonnull
+ List<Node> children();
+
+ /**
+ * Returns the remote repositories of this node.
+ *
+ * @return repositories of this node, never {@code null}
+ */
+ @Nonnull
+ List<RemoteRepository> remoteRepositories();
+
+ /**
+ * Returns the remote repository from which this artifact was downloaded, if known.
+ *
+ * @return an {@code Optional} containing the repository, or empty if not available (e.g. local artifact or root node)
+ */
+ @Nonnull
+ Optional<RemoteRepository> repository();
Review Comment:
The Javadoc was corrected in 45f2a0860a (the previous commit): `repository()` now documents `@return an Optional containing the repository, or empty if not available (e.g. local artifact or root node)`, which matches the upstream `getRepository()` wording.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/LocalRepository.java:
##########
@@ -46,6 +46,23 @@
@Immutable
public interface LocalRepository extends Repository {
+ /**
+ * Returns the path of this local repository.
+ *
+ * @return the path, never {@code null}
+ */
@Nonnull
- Path getPath();
+ Path path();
+
+ /**
+ * Returns the path of this local repository.
+ *
+ * @return the path, never {@code null}
+ * @deprecated Use {@link #path()} instead.
+ */
+ @Nonnull
Review Comment:
Fixed in f0701c6f49.
--
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] Switch core API value types to noun-style accessors [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13036:
URL: https://github.com/apache/maven/pull/13036#discussion_r4023885628
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Node.java:
##########
@@ -40,37 +40,98 @@
@Provider
public interface Node {
+ /**
+ * Returns the artifact for this node.
+ *
+ * @return artifact for this node, or {@code null} if none
+ */
+ @Nullable
+ Artifact artifact();
+
+ /**
+ * Returns the dependency for this node.
+ *
+ * @return dependency for this node, or {@code null} if none
+ */
+ @Nullable
+ Dependency dependency();
+
+ /**
+ * Gets the child nodes of this node.
+ *
+ * @return the child nodes of this node, never {@code null}
+ */
+ @Nonnull
+ List<Node> children();
+
+ /**
+ * Returns the remote repositories of this node.
+ *
+ * @return repositories of this node, never {@code null}
+ */
+ @Nonnull
+ List<RemoteRepository> remoteRepositories();
+
+ /**
+ * The repository where this artifact has been downloaded from.
+ *
+ * @return the repository, never {@code null}
+ */
+ @Nonnull
+ Optional<RemoteRepository> repository();
Review Comment:
⚠️ **Inaccurate Javadoc contract**: `@return the repository, never {@code null}` was added in this commit, but `DefaultNode.repository()` always throws `UnsupportedOperationException("Not implemented yet")`. The `never null` claim is factually wrong and will mislead callers who guard against `null` instead of catching the exception.
The previous `getRepository()` Javadoc had no `@return` at all — this new one introduced the false contract. It should either document the unimplemented state or be removed until the method is actually implemented:
```suggestion
/**
* The repository where this artifact has been downloaded from.
*
* @return an {@code Optional} containing the repository, or an empty Optional if not available
* @throws UnsupportedOperationException if not yet implemented by the provider
*/
@Nonnull
Optional<RemoteRepository> repository();
```
--
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] Switch core API value types to noun-style accessors [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13036:
URL: https://github.com/apache/maven/pull/13036#discussion_r3930271824
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ArtifactCoordinatesFactoryRequest.java:
##########
@@ -97,10 +97,10 @@ static ArtifactCoordinatesFactoryRequest build(@Nonnull Session session, @Nonnul
return ArtifactCoordinatesFactoryRequest.builder()
.session(requireNonNull(session, "session"))
.groupId(requireNonNull(coordinates, "coordinates").getGroupId())
Review Comment:
Fixed in e8c3ae3046.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/DependencyCoordinatesFactoryRequest.java:
##########
@@ -77,10 +77,10 @@ static DependencyCoordinatesFactoryRequest build(
.session(requireNonNull(session, "session cannot be null"))
.groupId(requireNonNull(coordinates, "coordinates cannot be null")
.getGroupId())
Review Comment:
Fixed in e8c3ae3046.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/DependencyCoordinatesFactoryRequest.java:
##########
@@ -89,12 +89,12 @@ static DependencyCoordinatesFactoryRequest build(@Nonnull Session session, @Nonn
return builder()
.session(requireNonNull(session, "session cannot be null"))
.groupId(requireNonNull(dependency, "dependency").getGroupId())
Review Comment:
Fixed in e8c3ae3046.
--
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] Switch core API value types to noun-style accessors [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13036:
URL: https://github.com/apache/maven/pull/13036#discussion_r4047348988
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Node.java:
##########
@@ -40,39 +40,98 @@
@Provider
public interface Node {
+ /**
+ * Returns the artifact for this node.
+ *
+ * @return artifact for this node, or {@code null} if none
+ */
+ @Nullable
+ Artifact artifact();
+
+ /**
+ * Returns the dependency for this node.
+ *
+ * @return dependency for this node, or {@code null} if none
+ */
+ @Nullable
+ Dependency dependency();
+
+ /**
+ * Gets the child nodes of this node.
+ *
+ * @return the child nodes of this node, never {@code null}
+ */
+ @Nonnull
+ List<Node> children();
+
+ /**
+ * Returns the remote repositories of this node.
+ *
+ * @return repositories of this node, never {@code null}
+ */
+ @Nonnull
+ List<RemoteRepository> remoteRepositories();
+
+ /**
+ * Returns the remote repository from which this artifact was downloaded, if known.
+ *
+ * @return an {@code Optional} containing the repository, or empty if not available (e.g. local artifact or root node)
+ */
+ @Nonnull
+ Optional<RemoteRepository> repository();
Review Comment:
⚠️ **Inaccurate Javadoc — NOT addressed from previous review.**
The Javadoc says `@return the repository, never {@code null}`, but:
1. The method returns `Optional<RemoteRepository>` — `never null` is misleading framing for an Optional (it implies a non-null value, not an Optional wrapper)
2. `DefaultNode.repository()` still throws `UnsupportedOperationException("Not implemented yet")` in the current PR HEAD (9c98b45)
PR #13153 was merged on 2026-09-16 and implemented `DefaultNode.repository()`, but this PR hasn't been rebased since — the implementation fix doesn't land here. More importantly, the Javadoc on `Node.repository()` itself was **downgraded** by this PR: master had `@return an Optional containing the repository, or empty if not available (e.g. local artifact or root node)` on `getRepository()`, which this commit replaced with the less accurate `@return the repository, never null`.
```suggestion
/**
* Returns the remote repository from which this artifact was downloaded, if known.
*
* @return an {@code Optional} containing the repository, or empty if not available (e.g. local artifact or root node)
*/
@Nonnull
Optional<RemoteRepository> repository();
```
##########
api/maven-api-core/src/main/java/org/apache/maven/api/LocalRepository.java:
##########
@@ -46,6 +46,23 @@
@Immutable
public interface LocalRepository extends Repository {
+ /**
+ * Returns the path of this local repository.
+ *
+ * @return the path, never {@code null}
+ */
@Nonnull
- Path getPath();
+ Path path();
+
+ /**
+ * Returns the path of this local repository.
+ *
+ * @return the path, never {@code null}
+ * @deprecated Use {@link #path()} instead.
+ */
+ @Nonnull
Review Comment:
🔹 **Nit — inconsistent annotation ordering.**
`@Nonnull` appears before `@Deprecated` here, while every other deprecated wrapper in this PR has `@Deprecated` first, `@Nonnull` second (e.g. `Artifact.getGroupId()`, `Artifact.getClassifier()`, etc.).
```suggestion
@Deprecated(since = "4.1.0", forRemoval = true)
@Nonnull
default Path getPath() {
return path();
}
```
--
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] Switch core API value types to noun-style accessors [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13036:
URL: https://github.com/apache/maven/pull/13036#discussion_r3930200150
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/DependencyCoordinatesFactoryRequest.java:
##########
@@ -77,10 +77,10 @@ static DependencyCoordinatesFactoryRequest build(
.session(requireNonNull(session, "session cannot be null"))
.groupId(requireNonNull(coordinates, "coordinates cannot be null")
.getGroupId())
Review Comment:
Same missed conversion — `getGroupId()` left as-is while the rest of the chain uses noun-style.
```suggestion
.groupId())
```
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ArtifactCoordinatesFactoryRequest.java:
##########
@@ -97,10 +97,10 @@ static ArtifactCoordinatesFactoryRequest build(@Nonnull Session session, @Nonnul
return ArtifactCoordinatesFactoryRequest.builder()
.session(requireNonNull(session, "session"))
.groupId(requireNonNull(coordinates, "coordinates").getGroupId())
Review Comment:
`getGroupId()` on the `requireNonNull()` return was not converted to `groupId()`, while `artifactId()`, `classifier()`, `versionConstraint()`, and `extension()` on the same object (next lines) were.
```suggestion
.groupId(requireNonNull(coordinates, "coordinates").groupId())
```
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/DependencyCoordinatesFactoryRequest.java:
##########
@@ -89,12 +89,12 @@ static DependencyCoordinatesFactoryRequest build(@Nonnull Session session, @Nonn
return builder()
.session(requireNonNull(session, "session cannot be null"))
.groupId(requireNonNull(dependency, "dependency").getGroupId())
Review Comment:
Same pattern — `getGroupId()` not converted.
```suggestion
.groupId(requireNonNull(dependency, "dependency").groupId())
```
--
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] Switch core API value types to noun-style accessors [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13036:
URL: https://github.com/apache/maven/pull/13036#discussion_r4047456639
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Node.java:
##########
@@ -40,39 +40,98 @@
@Provider
public interface Node {
+ /**
+ * Returns the artifact for this node.
+ *
+ * @return artifact for this node, or {@code null} if none
+ */
+ @Nullable
+ Artifact artifact();
+
+ /**
+ * Returns the dependency for this node.
+ *
+ * @return dependency for this node, or {@code null} if none
+ */
+ @Nullable
+ Dependency dependency();
+
+ /**
+ * Gets the child nodes of this node.
+ *
+ * @return the child nodes of this node, never {@code null}
+ */
+ @Nonnull
+ List<Node> children();
+
+ /**
+ * Returns the remote repositories of this node.
+ *
+ * @return repositories of this node, never {@code null}
+ */
+ @Nonnull
+ List<RemoteRepository> remoteRepositories();
+
+ /**
+ * Returns the remote repository from which this artifact was downloaded, if known.
+ *
+ * @return an {@code Optional} containing the repository, or empty if not available (e.g. local artifact or root node)
+ */
+ @Nonnull
+ Optional<RemoteRepository> repository();
+
/**
* @return artifact for this node
+ * @deprecated Use {@link #artifact()} instead.
*/
@Nullable
- Artifact getArtifact();
+ @Deprecated(since = "4.1.0", forRemoval = true)
+ default Artifact getArtifact() {
+ return artifact();
+ }
/**
* @return dependency for this node
+ * @deprecated Use {@link #dependency()} instead.
*/
@Nullable
- Dependency getDependency();
+ @Deprecated(since = "4.1.0", forRemoval = true)
+ default Dependency getDependency() {
+ return dependency();
+ }
/**
* Gets the child nodes of this node.
*
* @return the child nodes of this node, never {@code null}
+ * @deprecated Use {@link #children()} instead.
*/
@Nonnull
- List<Node> getChildren();
+ @Deprecated(since = "4.1.0", forRemoval = true)
+ default List<Node> getChildren() {
Review Comment:
Fixed in 9f4c9ab32a — all three `@Nonnull` wrappers (`getChildren`, `getRemoteRepositories`, `getRepository`) now have `@Deprecated` before `@Nonnull`. Also fixed `LocalRepository.getPath()` in the same commit.
--
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] Switch core API value types to noun-style accessors [maven]
Posted by "gnodet (via GitHub)" <gi...@apache.org>.
gnodet commented on code in PR #13036:
URL: https://github.com/apache/maven/pull/13036#discussion_r3930644377
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/ArtifactCoordinatesFactoryRequest.java:
##########
@@ -97,10 +97,10 @@ static ArtifactCoordinatesFactoryRequest build(@Nonnull Session session, @Nonnul
return ArtifactCoordinatesFactoryRequest.builder()
.session(requireNonNull(session, "session"))
.groupId(requireNonNull(coordinates, "coordinates").getGroupId())
Review Comment:
Fixed in e8c3ae3046.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/DependencyCoordinatesFactoryRequest.java:
##########
@@ -77,10 +77,10 @@ static DependencyCoordinatesFactoryRequest build(
.session(requireNonNull(session, "session cannot be null"))
.groupId(requireNonNull(coordinates, "coordinates cannot be null")
.getGroupId())
Review Comment:
Fixed in e8c3ae3046.
##########
api/maven-api-core/src/main/java/org/apache/maven/api/services/DependencyCoordinatesFactoryRequest.java:
##########
@@ -89,12 +89,12 @@ static DependencyCoordinatesFactoryRequest build(@Nonnull Session session, @Nonn
return builder()
.session(requireNonNull(session, "session cannot be null"))
.groupId(requireNonNull(dependency, "dependency").getGroupId())
Review Comment:
Fixed in e8c3ae3046.
--
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] Switch core API value types to noun-style accessors [maven]
Posted by "gnodet-bot (via GitHub)" <gi...@apache.org>.
gnodet-bot commented on code in PR #13036:
URL: https://github.com/apache/maven/pull/13036#discussion_r4047425995
##########
api/maven-api-core/src/main/java/org/apache/maven/api/Node.java:
##########
@@ -40,39 +40,98 @@
@Provider
public interface Node {
+ /**
+ * Returns the artifact for this node.
+ *
+ * @return artifact for this node, or {@code null} if none
+ */
+ @Nullable
+ Artifact artifact();
+
+ /**
+ * Returns the dependency for this node.
+ *
+ * @return dependency for this node, or {@code null} if none
+ */
+ @Nullable
+ Dependency dependency();
+
+ /**
+ * Gets the child nodes of this node.
+ *
+ * @return the child nodes of this node, never {@code null}
+ */
+ @Nonnull
+ List<Node> children();
+
+ /**
+ * Returns the remote repositories of this node.
+ *
+ * @return repositories of this node, never {@code null}
+ */
+ @Nonnull
+ List<RemoteRepository> remoteRepositories();
+
+ /**
+ * Returns the remote repository from which this artifact was downloaded, if known.
+ *
+ * @return an {@code Optional} containing the repository, or empty if not available (e.g. local artifact or root node)
+ */
+ @Nonnull
+ Optional<RemoteRepository> repository();
+
/**
* @return artifact for this node
+ * @deprecated Use {@link #artifact()} instead.
*/
@Nullable
- Artifact getArtifact();
+ @Deprecated(since = "4.1.0", forRemoval = true)
+ default Artifact getArtifact() {
+ return artifact();
+ }
/**
* @return dependency for this node
+ * @deprecated Use {@link #dependency()} instead.
*/
@Nullable
- Dependency getDependency();
+ @Deprecated(since = "4.1.0", forRemoval = true)
+ default Dependency getDependency() {
+ return dependency();
+ }
/**
* Gets the child nodes of this node.
*
* @return the child nodes of this node, never {@code null}
+ * @deprecated Use {@link #children()} instead.
*/
@Nonnull
- List<Node> getChildren();
+ @Deprecated(since = "4.1.0", forRemoval = true)
+ default List<Node> getChildren() {
Review Comment:
🔹 **Nit — inconsistent annotation ordering (3 wrappers).**
`@Nonnull` appears before `@Deprecated` here (and in `getRemoteRepositories`/`getRepository` below), while:
- The `@Nullable` wrappers in the same file (`getArtifact`, `getDependency`) correctly have `@Deprecated` first
- `Artifact.java` consistently uses `@Deprecated` then `@Nonnull`
- The just-committed fix for `LocalRepository.getPath()` enforces `@Deprecated` then `@Nonnull`
Same fix needed for all three `@Nonnull` wrappers:
```suggestion
@Deprecated(since = "4.1.0", forRemoval = true)
@Nonnull
default List<Node> getChildren() {
```
--
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