RFR: 8154482: javadoc tool must support legacy doclet and taglet

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

RFR: 8154482: javadoc tool must support legacy doclet and taglet

Kumar Srinivasan
Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8154482: javadoc tool must support legacy doclet and taglet

Kumar Srinivasan

Hello,

Based on the reviews here is the second revision of the fix.
http://cr.openjdk.java.net/~ksrini/8154482/webrev.01/


Highlights of the changes:

1.  Removed the redundant undocumented option -Xnew
2. A doclet on the command lines decides the target,
     irrespective of the taglet variant, this simplifies the logic.
3. Added a new annotation based testcase execution utility,
     in the toolbox package.
4. Added more testcases, refactored the test to use toolbox utilities,
    and other simplifications.

Thanks
Kumar



> Hello,
>
> Please review  fix for:
> https://bugs.openjdk.java.net/browse/JDK-8154482
>
> Webrev at:
> http://cr.openjdk.java.net/~ksrini/8154482/webrev.00/
>
> Thanks
> Kumar
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8154482: javadoc tool must support legacy doclet and taglet

Jonathan Gibbons
Start.java, line 241, 241, use &&, not nested if

Start:490,  generally, the terminology in langtools is to use "path"
to mean a complete search path, as in a series of file system locations,
such as a class path or source path.   With that in mind, the decl
on 490 would be better named "userTagletPath" (singular)
not "userTagletPaths" plural.

Yes, it gets confusing in parts of the code that use the NIO2 type
Path as a replacement for File.  There, the convention is to use
"search path" as the generalization of class path, source path, etc.

Start 598, 605, 616
The use of panic and its use of Messager.exit would be improved
by replacing
     panic(key, args)
     return false; // keep compiler happy
with either
     error(key, args)
     throw new ExitJavadoc();

(Ultimately, Start.exit and Messager.exit should go away.)

javadoc.properties.
118.   Ending a sentence "at the earliest." may be an acceptable
colloquialism, but guardians of the Queen's English would raise
an eyebrow or two.   I would just delete these words.  Yes, we
should recommend that folk migrate, but time is not of the
essence,


new test:
It's OK as it is, but the test-case names are not informative,
and may be tedious to update if you want  to add a new test case
in the middle of the list at some point in the future. Also, the
names are uninformative when they show up in a stack trace.
With this style of test case, I've typically found it useful to
use description names that describe the test case or test
conditions.

TestRunner should have class level javadoc. Thanks for creating
this class.

-- Jon



On 04/28/2016 10:48 AM, Kumar Srinivasan wrote:

>
> Hello,
>
> Based on the reviews here is the second revision of the fix.
> http://cr.openjdk.java.net/~ksrini/8154482/webrev.01/
>
>
> Highlights of the changes:
>
> 1.  Removed the redundant undocumented option -Xnew
> 2. A doclet on the command lines decides the target,
>     irrespective of the taglet variant, this simplifies the logic.
> 3. Added a new annotation based testcase execution utility,
>     in the toolbox package.
> 4. Added more testcases, refactored the test to use toolbox utilities,
>    and other simplifications.
>
> Thanks
> Kumar
>
>
>
>> Hello,
>>
>> Please review  fix for:
>> https://bugs.openjdk.java.net/browse/JDK-8154482
>>
>> Webrev at:
>> http://cr.openjdk.java.net/~ksrini/8154482/webrev.00/
>>
>> Thanks
>> Kumar
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8154482: javadoc tool must support legacy doclet and taglet

Kumar Srinivasan

Please review, this addresses all your comments below.
http://cr.openjdk.java.net/~ksrini/8154482/webrev.02/

Thanks
Kumar

> Start.java, line 241, 241, use &&, not nested if
>
> Start:490,  generally, the terminology in langtools is to use "path"
> to mean a complete search path, as in a series of file system locations,
> such as a class path or source path.   With that in mind, the decl
> on 490 would be better named "userTagletPath" (singular)
> not "userTagletPaths" plural.
>
> Yes, it gets confusing in parts of the code that use the NIO2 type
> Path as a replacement for File.  There, the convention is to use
> "search path" as the generalization of class path, source path, etc.
>
> Start 598, 605, 616
> The use of panic and its use of Messager.exit would be improved
> by replacing
>     panic(key, args)
>     return false; // keep compiler happy
> with either
>     error(key, args)
>     throw new ExitJavadoc();
>
> (Ultimately, Start.exit and Messager.exit should go away.)
>
> javadoc.properties.
> 118.   Ending a sentence "at the earliest." may be an acceptable
> colloquialism, but guardians of the Queen's English would raise
> an eyebrow or two.   I would just delete these words.  Yes, we
> should recommend that folk migrate, but time is not of the
> essence,
>
>
> new test:
> It's OK as it is, but the test-case names are not informative,
> and may be tedious to update if you want  to add a new test case
> in the middle of the list at some point in the future. Also, the
> names are uninformative when they show up in a stack trace.
> With this style of test case, I've typically found it useful to
> use description names that describe the test case or test
> conditions.
>
> TestRunner should have class level javadoc. Thanks for creating
> this class.
>
> -- Jon
>
>
>
> On 04/28/2016 10:48 AM, Kumar Srinivasan wrote:
>>
>> Hello,
>>
>> Based on the reviews here is the second revision of the fix.
>> http://cr.openjdk.java.net/~ksrini/8154482/webrev.01/
>>
>>
>> Highlights of the changes:
>>
>> 1.  Removed the redundant undocumented option -Xnew
>> 2. A doclet on the command lines decides the target,
>>     irrespective of the taglet variant, this simplifies the logic.
>> 3. Added a new annotation based testcase execution utility,
>>     in the toolbox package.
>> 4. Added more testcases, refactored the test to use toolbox utilities,
>>    and other simplifications.
>>
>> Thanks
>> Kumar
>>
>>
>>
>>> Hello,
>>>
>>> Please review  fix for:
>>> https://bugs.openjdk.java.net/browse/JDK-8154482
>>>
>>> Webrev at:
>>> http://cr.openjdk.java.net/~ksrini/8154482/webrev.00/
>>>
>>> Thanks
>>> Kumar
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: RFR: 8154482: javadoc tool must support legacy doclet and taglet

Jonathan Gibbons
OK, but one has to wonder why ExitJavadoc is defined in Messager, and not
at the top level.

-- Jon

On 04/29/2016 11:28 AM, Kumar Srinivasan wrote:

>
> Please review, this addresses all your comments below.
> http://cr.openjdk.java.net/~ksrini/8154482/webrev.02/
>
> Thanks
> Kumar
>
>> Start.java, line 241, 241, use &&, not nested if
>>
>> Start:490,  generally, the terminology in langtools is to use "path"
>> to mean a complete search path, as in a series of file system locations,
>> such as a class path or source path.   With that in mind, the decl
>> on 490 would be better named "userTagletPath" (singular)
>> not "userTagletPaths" plural.
>>
>> Yes, it gets confusing in parts of the code that use the NIO2 type
>> Path as a replacement for File.  There, the convention is to use
>> "search path" as the generalization of class path, source path, etc.
>>
>> Start 598, 605, 616
>> The use of panic and its use of Messager.exit would be improved
>> by replacing
>>     panic(key, args)
>>     return false; // keep compiler happy
>> with either
>>     error(key, args)
>>     throw new ExitJavadoc();
>>
>> (Ultimately, Start.exit and Messager.exit should go away.)
>>
>> javadoc.properties.
>> 118.   Ending a sentence "at the earliest." may be an acceptable
>> colloquialism, but guardians of the Queen's English would raise
>> an eyebrow or two.   I would just delete these words.  Yes, we
>> should recommend that folk migrate, but time is not of the
>> essence,
>>
>>
>> new test:
>> It's OK as it is, but the test-case names are not informative,
>> and may be tedious to update if you want  to add a new test case
>> in the middle of the list at some point in the future. Also, the
>> names are uninformative when they show up in a stack trace.
>> With this style of test case, I've typically found it useful to
>> use description names that describe the test case or test
>> conditions.
>>
>> TestRunner should have class level javadoc. Thanks for creating
>> this class.
>>
>> -- Jon
>>
>>
>>
>> On 04/28/2016 10:48 AM, Kumar Srinivasan wrote:
>>>
>>> Hello,
>>>
>>> Based on the reviews here is the second revision of the fix.
>>> http://cr.openjdk.java.net/~ksrini/8154482/webrev.01/
>>>
>>>
>>> Highlights of the changes:
>>>
>>> 1.  Removed the redundant undocumented option -Xnew
>>> 2. A doclet on the command lines decides the target,
>>>     irrespective of the taglet variant, this simplifies the logic.
>>> 3. Added a new annotation based testcase execution utility,
>>>     in the toolbox package.
>>> 4. Added more testcases, refactored the test to use toolbox utilities,
>>>    and other simplifications.
>>>
>>> Thanks
>>> Kumar
>>>
>>>
>>>
>>>> Hello,
>>>>
>>>> Please review  fix for:
>>>> https://bugs.openjdk.java.net/browse/JDK-8154482
>>>>
>>>> Webrev at:
>>>> http://cr.openjdk.java.net/~ksrini/8154482/webrev.00/
>>>>
>>>> Thanks
>>>> Kumar
>>>>
>>>
>>
>