RFR : 8219958 : Automatically load taglets from a jar file
Priya Lakshmi Muthuswamy
priya.lakshmi.muthuswamy at oracle.com
Wed Mar 20 03:38:13 UTC 2019
Hi,
When the ran the tests in mach5, I got an error in windows-x64 platform
(Error. failed to clean up files after test).
In all other environments its working fine.
windows is failing to delete the jar file .
So I have specified @run main/othervm for the test.
Thanks,
Priya
On 3/16/2019 6:40 AM, Jonathan Gibbons wrote:
>
> 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/20190320/8ca1a618/attachment-0001.html>
More information about the javadoc-dev
mailing list