Re: RFR : 8219958 : Automatically load taglets from a jar file

Thu, 14 Mar 2019 15:25:15 -0700

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

Reply via email to