RFR : 8219958 : Automatically load taglets from a jar file
Jonathan Gibbons
jonathan.gibbons at oracle.com
Thu Mar 14 22:21:56 UTC 2019
In the properties file, for doclet.not_standard_file_manager, change the
comma to a semicolon or colon.
In TagletManager,
line 207: no doc comment on the new method
line 245: fix capitalization "Loads taglets from a taglet path..."
line 247: @throws does not match actual "throws" and is empty
line 251: creating a spliterator to count the items on an iterable to
see if it is emprty is overkill, and neglects the possibility that the
iterable may be null. The if statement may not be strictly necessary,
but if it is, I recommend
251 if (location != null && location.iterator().hasNext()) {
line 263: the convention for javadoc comments on methods is to use 3rd
person declarative tense, not 2nd person imperative tense. In other
words, the verb should be "Registers".
In the test, there's a lot of duplicated text, generating the two source
files; I suggest using a common method, taking a String or int parameter
that is used to differentiate the instances.
While the test does load more than one taglet, that is not the defining
characteristic of the test; the defining characteristic is that the
taglets are loaded automatically, using ServiceLoader, instead of
explicitly, using command line args. I suggest you rename the test to
TestAutoLoadTaglets or something like that.
-- Jon
On 03/12/2019 04:33 AM, Priya Lakshmi Muthuswamy wrote:
>
> 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/20190314/0a12b546/attachment-0001.html>
More information about the javadoc-dev
mailing list