RFR : 8219958 : Automatically load taglets from a jar file
Jonathan Gibbons
jonathan.gibbons at oracle.com
Sat Mar 16 01:10:14 UTC 2019
TagletManager, line 230, change "Add" to "Adds".
Then you're OK.
-- Jon
On 03/14/2019 10:11 PM, Priya Lakshmi Muthuswamy wrote:
>
> 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/63100c35/attachment-0001.html>
More information about the javadoc-dev
mailing list