RFR: 8266666: Implementation for snippets [v25]

[Jonathan Gibbons]

On Mon, 20 Sep 2021 17:01:20 GMT, Pavel Rappo <prappo at openjdk.org> wrote:

>> This PR implements JEP 413 "Code Snippets in Java API Documentation", which hasn't been yet proposed to target JDK 18. The PR starts as a squashed merge of the https://github.com/openjdk/jdk-sandbox/tree/jdk.javadoc/snippets branch.
>
> 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

My primary concern in the last round of review was internationalization. You've addressed that: good.

When reading, and re-reading, a contribution this big, it's always possible to see new tiny details that could be improved. I've mentioned some here, and I'm sure that in times to come we will look at the code more and maybe see more tweaks. So, enough for now. Well done.

I note the varied references to improve JavadocTester.

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

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?

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"?

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

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java line 149:

> 147:                 {@snippet id=:
> 148:                     Hello, Snippet!
> 149:                 }

empty ids should be rejected.

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

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java line 608:

> 606:             failed("Looks like a stacktrace: " + matcher.group());
> 607:         }
> 608:     }

This could maybe be promoted to a default-enabled check in JavadocTester

test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java line 1201:

> 1199: 
> 1200:     // TODO: perhaps this method could be added to JavadocTester
> 1201:     private void checkOutputEither(Output out, String first, String... other) {

1. This method allows a call with just a single String arg.
2. Given that the method allows many String args, the name might be better as `checkOneOf`

Note to self/@jonathan-gibbons: review all references for suggestions to JavadocTester

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

Marked as reviewed by jjg (Reviewer).

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