bmarwell commented on code in PR #599:
URL:
https://github.com/apache/maven-jlink-plugin/pull/599#discussion_r2020233599
##########
src/main/java/org/apache/maven/plugins/jlink/JLinkMojo.java:
##########
@@ -464,12 +464,16 @@ public void execute() throws MojoExecutionException,
MojoFailureException {
}
}
- private List<File> getCompileClasspathElements(MavenProject project) {
+ List<File> getCompileClasspathElements(MavenProject project) {
List<File> list = new ArrayList<>(project.getArtifacts().size() + 1);
for (Artifact a : project.getArtifacts()) {
Review Comment:
`a` is a very short name for a variable. :)
##########
src/test/java/org/apache/maven/plugins/jlink/JLinkMojoTest.java:
##########
@@ -77,4 +82,24 @@ void single_quotes_shell_command_windows() throws Exception {
.contains(
"\\path\\to\\jlink \"--strip-debug\" \"--module-path\"
\"foo;bar\" \"--add-modules\" \"mvn,jlink");
}
+
+ @Test
+ void getCompileClasspathElements() throws Exception {
+ // Given
+ MavenProject project = Mockito.mock(MavenProject.class);
+
+ Artifact pomArtifact = Mockito.mock(Artifact.class);
+ when(pomArtifact.getType()).thenReturn("pom");
+
+ Artifact jarArtifact = Mockito.mock(Artifact.class);
+ when(jarArtifact.getType()).thenReturn("jar");
+ when(jarArtifact.getFile()).thenReturn(new File("some.jar"));
+
+ when(project.getArtifacts()).thenReturn(Set.of(pomArtifact,
jarArtifact));
+
+ List<File> classpathElements =
mojo.getCompileClasspathElements(project);
Review Comment:
Just curious, I haven't written tests in a while. Is there not an
implementation of Artifact on the class path we could use?
##########
src/main/java/org/apache/maven/plugins/jlink/JLinkMojo.java:
##########
@@ -464,12 +464,16 @@ public void execute() throws MojoExecutionException,
MojoFailureException {
}
}
- private List<File> getCompileClasspathElements(MavenProject project) {
+ List<File> getCompileClasspathElements(MavenProject project) {
List<File> list = new ArrayList<>(project.getArtifacts().size() + 1);
for (Artifact a : project.getArtifacts()) {
- getLog().debug("Artifact: " + a.getGroupId() + ":" +
a.getArtifactId() + ":" + a.getVersion());
- list.add(a.getFile());
+ boolean skip = "pom".equals(a.getType());
+ getLog().debug("Adding artifact: " + a.getGroupId() + ":" +
a.getArtifactId() + ":" + a.getVersion()
+ + (skip ? " (skipping)" : ""));
+ if (!skip) {
+ list.add(a.getFile());
+ }
Review Comment:
Thinking out loudly, maybe in the future more deps will be skipped. Maybe
(just maybe) refactor out a method here which currently only contains
`"pom".equals(a.getType())`, e.g. `shouldSkip(artifact)`. Then rename `skip` to
`shouldSkip`.
Makes it future proof, and the renaming of the variable will tell
maintainers why it should be skipped (at least a little).
--
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]
