[MJAVADOC-775] Option 'taglets/taglet/tagletpath' ignored when pointing to a JAR #255
Conversation
src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
Outdated
Show resolved
Hide resolved
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
src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
Outdated
Show resolved
Hide resolved
src/main/java/org/apache/maven/plugins/javadoc/JavadocUtil.java
Outdated
Show resolved
Hide resolved
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. |
michael-o
force-pushed
the
taglet-path-fixes
branch
from
daf8514
to
66240c1
Compare
michael-o
changed the title
…ng to a JAR New method JavadocUtil::prunePaths takes care of pruning classpath elements correctly, the existing JavadocUtil::pruneDirs is now just a special case delegating to prunePaths. This closes apache#255
michael-o
force-pushed
the
taglet-path-fixes
branch
from
66240c1
to
91369fa
Compare
There was a problem hiding this comment.
This is good enough for me now, will merge as soon as CI passes.
Note that I have lifted the restrictions on JAR files for two reasons:
- We don't filter with
tagletpath, it should be consistent - ZIP files are first class classpath elements as well, so we allow everything and the rest is the responsiblity of the developer
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.