RFR: 8266666: Implementation for snippets [v25]

[Pavel Rappo]

On Tue, 21 Sep 2021 01:33:09 GMT, Jonathan Gibbons <jjg at openjdk.org> wrote:

>> Pavel Rappo has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 37 commits:
>> 
>>  - Merge branch 'master' into 8266666
>>  - Improve markup error messages
>>    
>>     * Cleans up error string format
>>     * Internationalizes error messages, mapping them to fewer resource keys
>>     * Fixes a few bugs related to error messages, including: inaccurate caret positioning and runaway resource name (i.e. the resource key wasn't translated into an error)
>>  - Merge branch 'master' into 8266666
>>  - Address trivial feedback
>>  - Address feedback
>>    
>>    1. Reverts changes from CommentUtils.java (unused methods)
>>    2. Changes misguided regex-related assertion to tests
>>  - Remove trailing whitespace to satisfy jcheck
>>  - Clean up code
>>    
>>    Removes stale FIXMEs, TODOs and comments; downgrades FIXMEs to TODOs where possible; wraps overly long lines.
>>  - Merge branch 'master' into 8266666
>>  - Merge branch 'master' into 8266666
>>  - Refactor SnippetTaglet
>>    
>>    Improves code style; reformats.
>>  - ... and 27 more: https://git.openjdk.java.net/jdk/compare/4b3a4fff...edb7cf85
>
> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/resources/doclets.properties line 400:
> 
>> 398: # 0: path; 1: exception
>> 399: doclet.error_setting_snippet_path=\
>> 400:   Error setting snippet path {0}: {1}
> 
> Add newline at end of file

Although it wasn't there to begin with, adding it won't be as big a deal as reformatting code in other places you suggested earlier.

> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/MarkupParser.java line 220:
> 
>> 218:         switch (ch) {
>> 219:             case ':': // indicates that the instruction relates to the next line
>> 220:             case ' ': case '\t':
> 
> What about other whitespace? Is that already covered?

Which other whitespace? If you are talking about `\r` or `\n`, then the string parsed by `MarkupParser` cannot have those. `MarkupParser` gets a single line without a trailing line terminator.

> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/ParseException.java line 35:
> 
>> 33:  * This exception is only used to capture a user-facing error message.
>> 34:  * The message supplier is accepted not to control when to obtain a message,
>> 35:  * but to abstract how to obtain in.
> 
> typo "in"?  maybe "one" or "it"?

Good catch! It should've been "it".

> test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java line 88:
> 
>> 86:     /*
>> 87:      * While the "id" and "lang" attributes are advertised in JEP 413, they are
>> 88:      * currently unused by the implementation. The goal of this test is to make
> 
> id's should be propagated to the output HTML

About empty id's and propagating id's to the output HTML. I think we can address these in a follow-up issue. Would you be okay with it?

> test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java line 473:
> 
>> 471:         // out of order with respect to the errors checked above.
>> 472:         // This is because the below errors are modelled as exceptions thrown
>> 473:         // at parse time, when there are no doc trees yet. And the above errors
> 
> This would read better as "the errors below" and "the errors above"
> https://www.grammarphobia.com/blog/2012/12/below.html

Thanks.

-------------

PR: https://git.openjdk.java.net/jdk/pull/4795