RFR : 8219958 : Automatically load taglets from a jar file
Priya Lakshmi Muthuswamy
priya.lakshmi.muthuswamy at oracle.com
Fri Mar 15 05:11:42 UTC 2019
Hi Jon,
updated the webrev with suggested comments.
webrev : http://cr.openjdk.java.net/~pmuthuswamy/8219958/webrev.02/
Thanks,
Priya
On 3/15/2019 3:51 AM, Jonathan Gibbons wrote:
>
> 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/20190315/93643afd/attachment-0001.html>
More information about the javadoc-dev
mailing list