On 03/06/2019 02:56 AM, Priya Lakshmi Muthuswamy wrote:
Hi,
Kindly review the changes for automatically loading taglets from
a service provider jar.
JBS: https://bugs.openjdk.java.net/browse/JDK-8219958
webrev: http://cr.openjdk.java.net/~pmuthuswamy/8219958/webrev.00/
CSR: https://bugs.openjdk.java.net/browse/JDK-8219959
Thanks,
Priya
Priya,
*BaseConfiguration:*
You've moved the filemanager setup from TagletManager:216-226
into BaseConfiguration.
I agree it needs to be moved from its current position, but I
think it should stay in TagletManager.
I suggest that you create a new method in TagletManager to init
the taglet path.
The beginning of BaseConfiguration.initTagletManager would then
be something like:
837 tagletManager = tagletManager == null ?
838 new TagletManager(nosince, showversion, showauthor,
javafx, this) :
839 tagletManager;
tagletManager.initTagletPath(tagletPath);
840 for (List<String> args : customTagStrs) {
841 if (args.get(0).equals("-taglet")) {
842 tagletManager.addCustomTag(args.get(1), getFileManager(),
tagletpath);
843 continue;
844 }
As part of the suggestion to move stuff into BaseConfiguration,
you had suggested using
`try ... catch (Exception e)`. That would have been overly
broad; if we were to continue
to do that, it would have been better to narrow the caught
exception class to IOException.
*doclets.properties:*
The new error message needs improving. Think of how it would
appear to an end-user:
/Error: could not set location for /some/path/input/by/the/user/
What is that going to mean to a user? You also don't give any
additional diagnostic information
that might be of assistance to remedy the situation. I don't like
having to resort to using `e.toString()`
but when that's the only option, it's all we can do, and better
than nothing.
I suggest the message should be something like:
/
//Error: could not set the taglet path: {0}/
where the provided value is the exception message.
*
**TagletManager.java:*
I've already suggested that you should move the init of the
taglet path back into TagletManager.
The init code on lines 216-226 is somewhat curious, in that it
gives precedence to the TAGLET_PATH
in the file manager if it has been set ... that may be because
the code was executed repeatedly,
because addCustomTag is called in a loop.
We should refactor the init, to honor the taglet path option if
it is set, and the TAGLET_PATH if it
is set when calling javadoc through the javax.tools API.
Put that all together, and the initTagletPath code should look
something like the following (in pseudo-code)
void initTagletManager(String tagletPath) {
if (fileManager instanceof StandardJavaFileManager) {
Standard JavaFileManager sfm =
(StandardJavaFileManager) fileMaanger;
if (tagletPath != null) {
sfm.setLocation(TAGLET_PATH,
tagletPath.splitAsStream(File.pathSeparator).collect(Collectors.toList()));
} else if (!sfm.hasLocation(TAGLET_PATH)) {
sfm.setLocation(TAGLET_PATH,
Collections.emptyList());
}
} else if (tagletPath != null) {
messages.error("cannot set taglet path; the file
manager is not a StandardJavaFileManager");
}
}
In addCustomTag, what exceptions are you catching? It may be that
you can just catch
ReflectiveOperationException instead of Exception. If there are
other exceptions being thrown,
consider using a multi-catch.
In loadTagletsJar ... the method name is weird as well as the doc
comments.
I suggest ...
* calling the method "loadTaglets" or "loadRegisteredTaglets"
or something like that.
* the javadoc should be "Loads taglets from the taglet path
using ServiceLoader."
Remove the parameter "tagletPath" and remove the "if (tagletPath
!= null)" surrounding the
use of ServiceLoader. There is no need for the user to use
-tagletpath ... the user could be
calling javadoc through the javax.tools API with a preconfigured
filemanager that already
has the TAGLET_PATH set up. Also, it would be better to split
lines 234,235 after the '='
instead of before the '(' to avoid splitting the method call.
234 ServiceLoader<jdk.javadoc.doclet.Taglet> serviceLoader =
235 fileManager.getServiceLoader(TAGLET_PATH,
jdk.javadoc.doclet.Taglet.class);
In registerTaglet, you should not need to pass in the class name,
since it is just the class name
of the instance parameter, isn't it?
Also in registerTaglet, although you haven't modified it, the
body seems excessively verbose.
How about this:
245 /**
246 * Registers the Taglet.
247 * @param instance the Taglet instance
249 */
250 private void registerTaglet(jdk.javadoc.doclet.Taglet instance) {
251 instance.init(docEnv, doclet);
252 Taglet newLegacy = new UserTaglet(instance);
258 allTaglets.put(newLegacy.getName(), newLegacy);
259 messages.notice("doclet.Notice_taglet_registered",
instance.getClass().getName());
260 }
It's pretty icky having two classes with simple name Taglet,
forcing the use of fully-qualified
names. At some point (not necessarily in this review), we should
rename the internal one
to something else. (We can't change the name of the one in the
public API.)
-- Jon
