Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[MJAVADOC-783] Taglet path fixes #255

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

[MJAVADOC-783] Taglet path fixes #255

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6b45a44
MJAVADOC-783 Fix taglet path concatenation
kriegaex Dec 13, 2023
daf8514
MJAVADOC-775 Permit JARs when using taglets/taglet/tagletpath
kriegaex Dec 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/main/java/org/apache/maven/plugins/javadoc/AbstractJavadocMojo.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2946,8 +2946,9 @@ private String getTagletPath() throws MavenReportException {
&& (StringUtils.isNotEmpty(taglet.getTagletArtifact().getVersion()))) {
pathParts.addAll(JavadocUtil.pruneFiles(getArtifactsAbsolutePath(taglet.getTagletArtifact())));
} else if (StringUtils.isNotEmpty(taglet.getTagletpath())) {
for (Path dir : JavadocUtil.pruneDirs(project, Collections.singletonList(taglet.getTagletpath()))) {
pathParts.add(dir.toString());
for (Path path :
JavadocUtil.prunePaths(project, Collections.singletonList(taglet.getTagletpath()), true)) {
pathParts.add(path.toString());
}
}
}
Expand All @@ -2956,7 +2957,7 @@ private String getTagletPath() throws MavenReportException {
path.append(StringUtils.join(pathParts.iterator(), File.pathSeparator));

if (tagletpath != null && !tagletpath.isEmpty()) {
path.append(JavadocUtil.unifyPathSeparator(tagletpath));
path.append(path.length() > 0 ? File.pathSeparator : "").append(JavadocUtil.unifyPathSeparator(tagletpath));
}

return path.toString();
Expand Down
47 changes: 35 additions & 12 deletions src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
import java.util.Collections;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Locale;
import java.util.NoSuchElementException;
import java.util.Properties;
import java.util.Set;
Expand Down Expand Up @@ -118,32 +119,54 @@ public class JavadocUtil {
+ "environment variable using -Xms:<size> and -Xmx:<size>.";

/**
* Method that removes the invalid directories in the specified directories. <b>Note</b>: All elements in
* <code>dirs</code> could be an absolute or relative against the project's base directory <code>String</code> path.
* Method that removes invalid classpath elements in the specified paths. <b>Note</b>: All elements in
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does invalid mean in this context? Rewrite Javadoc to clarify

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you asking questions about existing comments in your product, which I have just copied/moved and slightly adjusted? If it was good enough so far, it should be good enough now.

* <code>paths</code> could be absolute or relative against the project's base directory <code>String</code> path.
* When pruning class paths, you can optionally include "*.jar" files in the result, otherwise only directories are
* permitted.
*
* @param project the current Maven project not null
* @param dirs the collection of <code>String</code> directories path that will be validated.
* @return a List of valid <code>String</code> directories absolute paths.
* @param paths the collection of <code>String</code> paths that will be validated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the collection of String paths --> the paths
Javadoc will insert the types

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you asking questions about existing comments in your product, which I have just copied/moved and slightly adjusted? If it was good enough so far, it should be good enough now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you copied/moved and slightly adjusted them. You also added new emthods that weren't here before.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't get it, do you? Then have your way, I could not care less. If you cannot see that this is PR is an improvement and refuse to accept it, you are just shooting in your own foot.

* @param includeJars whether to include JAR files in the result
* @return a list of valid <code>String</code> directories and optionally JAR files as absolute paths
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does valid mean here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you asking questions about existing comments in your product, which I have just copied/moved and slightly adjusted? If it was good enough so far, it should be good enough now.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was no prunePaths method here before. If you're adding a public method, clear documentation is required.

Copy link
Author

@kriegaex kriegaex Dec 13, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look, I understand if you are very busy and do not have enough time to triage bugs or review code. But if you cannot see the difference between a new method and a renamed, slightly extended one to convey the more generic behaviour, differentiating it by name from the original method the code was factored out from, I really cannot help you. I can live without you consenting to and merging these two bugfixes, if you want to coerce me to do your job to refactor the existing documentation, after I already did your job of fixing two old bugs. Merge it, fine. Refactor the docs after the merge, be my guest. Or don't merge it, hurting yourself and your users, also fine. I am done here. I am not having your nitpicking, micro-managing code reviews.

*/
public static Collection<Path> pruneDirs(MavenProject project, Collection<String> dirs) {
public static Collection<Path> prunePaths(MavenProject project, Collection<String> paths, boolean includeJars) {
final Path projectBasedir = project.getBasedir().toPath();

Set<Path> pruned = new LinkedHashSet<>(dirs.size());
for (String dir : dirs) {
if (dir == null) {
Set<Path> pruned = new LinkedHashSet<>(paths.size());
for (String path : paths) {
if (path == null) {
continue;
}

Path directory = projectBasedir.resolve(dir);

if (Files.isDirectory(directory)) {
pruned.add(directory.toAbsolutePath());
Path resolvedPath = projectBasedir.resolve(path);

if (Files.isDirectory(resolvedPath)
|| includeJars
&& Files.isRegularFile(resolvedPath)
&& resolvedPath
.getFileName()
.toString()
.toLowerCase(Locale.ROOT)
.endsWith(".jar")) {
pruned.add(resolvedPath.toAbsolutePath());
}
}

return pruned;
}

/**
* Method that removes the invalid directories in the specified directories. <b>Note</b>: All elements in
* <code>dirs</code> could be absolute or relative against the project's base directory <code>String</code> path.
*
* @param project the current Maven project not null
* @param dirs the collection of <code>String</code> directories that will be validated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the collection of String directories --> the paths of the directories
are these relative or absolute?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you asking questions about existing comments in your product, which I have just copied/moved and slightly adjusted? If it was good enough so far, it should be good enough now.

* @return a list of valid <code>String</code> directories as absolute paths
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does valid mean here?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you asking questions about existing comments in your product, which I have just copied/moved and slightly adjusted? If it was good enough so far, it should be good enough now.

*/
public static Collection<Path> pruneDirs(MavenProject project, Collection<String> dirs) {
return prunePaths(project, dirs, false);
}

/**
* Method that removes the invalid files in the specified files. <b>Note</b>: All elements in <code>files</code>
* should be an absolute <code>String</code> path.
Expand Down
37 changes: 33 additions & 4 deletions src/test/java/org/apache/maven/plugins/javadoc/JavadocUtilTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,11 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;
Expand Down Expand Up @@ -582,18 +584,45 @@ public void testCopyJavadocResources() throws Exception {
*/
public void testPruneDirs() {
List<String> list = new ArrayList<>();
list.add(getBasedir() + "/target/classes");
list.add(getBasedir() + "/target/classes");
list.add(getBasedir() + "/target/classes");
String classesDir = getBasedir() + "/target/classes";
list.add(classesDir);
list.add(classesDir);
list.add(classesDir);

Set<Path> expected = Collections.singleton(Paths.get(getBasedir(), "target/classes"));
Set<Path> expected = Collections.singleton(Paths.get(classesDir));

MavenProjectStub project = new MavenProjectStub();
project.setFile(new File(getBasedir(), "pom.xml"));

assertEquals(expected, JavadocUtil.pruneDirs(project, list));
}

/**
* Method to test prunePaths()
*
*/
public void testPrunePaths() {
List<String> list = new ArrayList<>();
String classesDir = getBasedir() + "/target/classes";
String tagletJar = getBasedir()
+ "/target/test-classes/unit/taglet-test/artifact-taglet/org/tullmann/taglets/1.0/taglets-1.0.jar";
list.add(classesDir);
list.add(classesDir);
list.add(classesDir);
list.add(tagletJar);
list.add(tagletJar);
list.add(tagletJar);

Set<Path> expectedNoJar = Collections.singleton(Paths.get(classesDir));
Set<Path> expectedWithJar = new HashSet<>(Arrays.asList(Paths.get(classesDir), Paths.get(tagletJar)));

MavenProjectStub project = new MavenProjectStub();
project.setFile(new File(getBasedir(), "pom.xml"));

assertEquals(expectedNoJar, JavadocUtil.prunePaths(project, list, false));
assertEquals(expectedWithJar, JavadocUtil.prunePaths(project, list, true));
}

/**
* Method to test unifyPathSeparator()
*
Expand Down