RFR : 8219958 : Automatically load taglets from a jar file
Priya Lakshmi Muthuswamy
priya.lakshmi.muthuswamy at oracle.com
Tue Mar 12 11:33:14 UTC 2019
Hi Jon,
I have updated the webrev with the suggested changes.
In TagletManager.java, LoadTaglets method, have removed the condition on
tagletpath, instead applied
check on filemanager.getLocation.
updated webrev: http://cr.openjdk.java.net/~pmuthuswamy/8219958/webrev.01/
Thanks,
Priya
On 3/9/2019 5:58 AM, Jonathan Gibbons wrote:
>
>
>
> 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.java.net/pipermail/javadoc-dev/attachments/20190312/44858b17/attachment-0001.html>
More information about the javadoc-dev
mailing list