Copilot commented on code in PR #394:
URL:
https://github.com/apache/maven-antrun-plugin/pull/394#discussion_r4134653347
##########
src/main/java/org/apache/maven/ant/tasks/DependencyFilesetsTask.java:
##########
@@ -62,8 +63,9 @@ public void execute() {
FileSet dependenciesFileSet = new FileSet();
dependenciesFileSet.setProject(getProject());
- ArtifactRepository localRepository =
getProject().getReference("maven.local.repository");
- dependenciesFileSet.setDir(new File(localRepository.getBasedir()));
+ LocalRepository localRepository =
getProject().getReference("maven.local.repository");
+ LocalRepositoryManager localRepositoryManager =
getProject().getReference("maven.local.repository.manager");
Review Comment:
The Ant reference IDs are duplicated here as string literals, and one of
them (`maven.local.repository.manager`) is newly introduced. To avoid drift
between producer (`AntRunMojo`) and consumer (`DependencyFilesetsTask`),
consider centralizing these keys (e.g., `public static final` constants in a
shared class) and referencing the constants from both locations.
##########
src/main/java/org/apache/maven/plugins/antrun/AntRunMojo.java:
##########
@@ -424,8 +436,12 @@ public void copyProperties(MavenProject mavenProject,
Project antProject) {
antProject.setProperty(
(propertyPrefix + "project.build.testSourceDirectory"),
mavenProject.getBuild().getTestSourceDirectory());
- antProject.setProperty((propertyPrefix + "localRepository"),
localRepository.toString());
- antProject.setProperty((propertyPrefix + "settings.localRepository"),
localRepository.getBasedir());
+ antProject.setProperty(
+ (propertyPrefix + "localRepository"),
+ getLocalRepository().getBasedir().getAbsolutePath());
Review Comment:
This line changes `${localRepository}` semantics from a debug `toString()`
dump to an absolute filesystem path. Since this is externally observable in Ant
builds, it should be covered by an integration/unit test assertion (e.g., that
`${localRepository}` equals `${settings.localRepository}` and points to an
existing directory) to prevent accidental regression.
##########
src/main/java/org/apache/maven/plugins/antrun/AntRunMojo.java:
##########
@@ -350,7 +360,9 @@ private void addAntProjectReferences(MavenProject
mavenProject, Project antProje
antProject.addReference(DEFAULT_MAVEN_PROJECT_REFID, mavenProject);
antProject.addReference(DEFAULT_MAVEN_PROJECT_REF_REFID, new
MavenAntRunProject(mavenProject));
antProject.addReference(DEFAULT_MAVEN_PROJECT_HELPER_REFID,
projectHelper);
- antProject.addReference(MAVEN_REFID_PREFIX + "local.repository",
localRepository);
+ antProject.addReference(MAVEN_REFID_PREFIX + "local.repository",
getLocalRepository());
Review Comment:
This changes the runtime type of the `maven.local.repository` Ant reference
from `ArtifactRepository` to `LocalRepository`, which will cause
`ClassCastException` in custom Ant tasks or builds that still cast that
reference to `ArtifactRepository`. To reduce the breaking impact, consider
publishing an additional (new) reference key for the resolver types (e.g.,
`maven.local.repository.resolver` / `...manager`) while keeping
`maven.local.repository` as a backward-compatible value (such as the local repo
basedir `File`), or add a deprecated compatibility shim cycle (even if
internally backed by resolver APIs).
##########
src/main/java/org/apache/maven/plugins/antrun/AntRunMojo.java:
##########
@@ -424,8 +436,12 @@ public void copyProperties(MavenProject mavenProject,
Project antProject) {
antProject.setProperty(
(propertyPrefix + "project.build.testSourceDirectory"),
mavenProject.getBuild().getTestSourceDirectory());
- antProject.setProperty((propertyPrefix + "localRepository"),
localRepository.toString());
- antProject.setProperty((propertyPrefix + "settings.localRepository"),
localRepository.getBasedir());
+ antProject.setProperty(
+ (propertyPrefix + "localRepository"),
+ getLocalRepository().getBasedir().getAbsolutePath());
+ antProject.setProperty(
+ (propertyPrefix + "settings.localRepository"),
+ getLocalRepository().getBasedir().getAbsolutePath());
Review Comment:
The local-repo absolute path is computed twice. Capture
`getLocalRepository().getBasedir().getAbsolutePath()` into a local variable (or
compute `LocalRepository` once) and reuse it for both properties to avoid
duplication and make future edits less error-prone.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
