[MJAVADOC-783] Taglet path fixes #255
Conversation
by adding missing path separator. Co-authored-by: Rob Gordon <rob@rgordon.co.uk>
src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
Outdated
Show resolved
Hide resolved
New method JavadocUtil::prunePaths takes care of pruning classpaths correctly, the existing JavadocUtil::pruneDirs is now just a special case delegating to prunePaths.
kriegaex
force-pushed
the
taglet-path-fixes
branch
from
ec8afb5
to
daf8514
Compare
|
@kriegaex Thank you for the PR. I first need to understand the original reports and your change whether I can computer this reasonably. It will take a few days. Tell me please, is an IT required here in this case or is the UT enough? |
|
@michael-o, you can find detailed information and reproducers in the two linked Jira issues. As for ITs, taglet paths are completely untested. At least, I did not find anything there. The string "taglet" does not occur at all in the IT directory. I think, ITs would make sense in general. But my PR does not make IT coverage any worse than zero. As for unit tests, I added a test for the new method. As one of the issues was closed after so-called "triage" without proper investigation, just because the two sounded similar, I was concerned that this topic would not be taken seriously by the product maintainers. So, I contributed this PR, which was not my original plan. I cannot fix all issues I encounter in all OSS products by myself, always dealing with code I have never seen before, and on top of it also improve test coverage. My own projects are already suffering more than is good for them. So please, let us not blow up the scope of this PR. But by all means, feel free to add ITs, committing on top of my changes after the merge. 🙂 |
If you consider both JIRA issues distinct although the fix is identical, I can reopen the closed one, no problem. |
|
The fix is not identical. There are two separate commits in this PR, fixing two distinct problems. Please read the commit comments, I referenced one issue per commit. I just did not want to create two PRs, because my time is limited, and I did not want to rebase later. |
My bad. |
elharo
changed the title
| * <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 |
There was a problem hiding this comment.
the collection of String directories --> the paths of the directories
are these relative or absolute?
There was a problem hiding this comment.
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.
| * | ||
| * @param project the current Maven project not null | ||
| * @param dirs the collection of <code>String</code> directories that will be validated | ||
| * @return a list of valid <code>String</code> directories as absolute paths |
There was a problem hiding this comment.
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 absolute paths. | ||
| * @param paths the collection of <code>String</code> paths that will be validated | ||
| * @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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
There was no prunePaths method here before. If you're adding a public method, clear documentation is required.
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
What does invalid mean in this context? Rewrite Javadoc to clarify
There was a problem hiding this comment.
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.
| * | ||
| * @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 |
There was a problem hiding this comment.
the collection of String paths --> the paths
Javadoc will insert the types
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Because you copied/moved and slightly adjusted them. You also added new emthods that weren't here before.
There was a problem hiding this comment.
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.
You did not know before either. Like I said, feel free to write an IT. Do not abuse contributors to catch up with work that was not done when it was due for existing code. |
@elharo, if you did not notice by inspecting my PR: I have just contributed something valuable, fixing two bugs within 30 minutes of seeing this code for the first time in my life. It is meant to be a minimally invasive change, not a structural refactoring of either code or javadoc. Hence, I kept wording and structure of the existing javadocs intact. Otherwise, I would have had to rewrite the javadocs for the whole class and maybe also for the rest of the module. FYI, I also checked whom you can ask what this javadoc means and why it was written that way: Vincent Siveton. I do not like style and wording either, but that was not the focus of this PR. In Vincent's defence, if you also look at the code, it becomes pretty clear what he means, even though probably you and I would have written the docs differently. There is a software craftsmanship principle which is often forgotten: to separate refactoring from functional changes. This PR is about functional changes and not supposed to restructure the code or its documentation more than necessary to accomodate to the fact that I made one method more flexible by an additional parameter and call that method from the previously existing one in to avoid code duplication. |
@elharo, I forgot to mention a detail: When you start writing ITs and want to keep the build running on both JDK 8 and JDK 9+ (CI builds currently run on 8 and 17), enjoy implementing all sample taglets twice, once for the old Doclet API and again for the new Doclet API on JDK 9+, see JEP 221. You can have the same fun I had when helping out a bit at Xalan-Java, creating JDK-dependent Java profiles adding a JDK 8 taglet module for the old API and a JDK 9+ one for the new API. All of this only to have sample code for ITs. Xalan-Java at least uses the doclets for their own javadocs. Of course, it makes sense to have such ITs, I am not denying that at all. But it is way off limits for this bugfix PR. Before anyone creates such ITs, the Maven Javadoc team should maybe first decide, if the small benefit of being able to still build (not run, only build!) the plugin on JDK 8 is worth the more than duplicate (because of extra profiles and distinction of cases) effort of creating and maintaining such ITs. After that decision, work on ITs can start in a separate PR. |
Fixes MJAVADOC-775.
Fixes MJAVADOC-783.
@elharo, @robjg, @jkesselm
To make clear that you license your contribution under
the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.
I hereby declare this contribution to be licensed under the Apache License Version 2.0, January 2004
In any other case, please file an Apache Individual Contributor License Agreement.