Conversation
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. Fixes #13035
46f6cdb to
c0f25d3
Compare
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Inconsistent getGroupId() calls left unconverted
The migration is clean and mechanically consistent across 9 interfaces and all implementations. One minor gap: three getGroupId() calls in the factory request classes were missed while adjacent accessors on the same object were converted to noun-style.
This review was generated by an AI agent, Hermès, on behalf of @gnodet.
| @@ -97,10 +97,10 @@ static ArtifactCoordinatesFactoryRequest build(@Nonnull Session session, @Nonnul | |||
| return ArtifactCoordinatesFactoryRequest.builder() | |||
| .session(requireNonNull(session, "session")) | |||
| .groupId(requireNonNull(coordinates, "coordinates").getGroupId()) | |||
There was a problem hiding this comment.
getGroupId() on the requireNonNull() return was not converted to groupId(), while artifactId(), classifier(), versionConstraint(), and extension() on the same object (next lines) were.
| .groupId(requireNonNull(coordinates, "coordinates").getGroupId()) | |
| .groupId(requireNonNull(coordinates, "coordinates").groupId()) |
| @@ -77,10 +77,10 @@ static DependencyCoordinatesFactoryRequest build( | |||
| .session(requireNonNull(session, "session cannot be null")) | |||
| .groupId(requireNonNull(coordinates, "coordinates cannot be null") | |||
| .getGroupId()) | |||
There was a problem hiding this comment.
Same missed conversion — getGroupId() left as-is while the rest of the chain uses noun-style.
| .getGroupId()) | |
| .groupId()) |
| @@ -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()) | |||
There was a problem hiding this comment.
Same pattern — getGroupId() not converted.
| .groupId(requireNonNull(dependency, "dependency").getGroupId()) | |
| .groupId(requireNonNull(dependency, "dependency").groupId()) |
gnodet
left a comment
gnodet
left a comment
There was a problem hiding this comment.
Re-review after e8c3ae3: the three getGroupId() calls flagged in the previous review are now converted to groupId(). Migration is mechanically consistent across all 9 interfaces, all implementations, all tests, and all internal callers. No remaining unconverted calls found.
LGTM — no issues.
This review was generated by an AI agent, Hermès, on behalf of @gnodet.
…nd LocalRepository
gnodet-bot
left a comment
gnodet-bot
left a comment
There was a problem hiding this comment.
Re-review after 9c98b45: migration of Node, Repository, RemoteRepository, LocalRepository to noun-style accessors is mechanically consistent — all 4 interfaces converted, all implementations (DefaultNode, WrapperNode, AbstractNode, DefaultLocalRepository, DefaultRemoteRepository, AbstractSession anonymous WorkspaceRepository, DefaultTransportProvider, DefaultModelBuilder) updated, all internal callers and tests updated. One Javadoc accuracy issue in the new commit.
This review was generated by an AI agent, Hermès, on behalf of @gnodet.
| * @return the repository, never {@code null} | ||
| */ | ||
| @Nonnull | ||
| Optional<RemoteRepository> repository(); |
There was a problem hiding this comment.
@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:
| Optional<RemoteRepository> repository(); | |
| /** | |
| * 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(); |
There was a problem hiding this 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.
2 participants
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