RFR: JDK-8275788: Create code element with suitable attributes for code snippets [v5]

[Pavel Rappo]

On Tue, 2 Nov 2021 18:54:48 GMT, Hannes Wallnöfer <hannesw at openjdk.org> wrote:

>> Please review a change to add a nested `<code>` element for snippet tags as well as HTML attributes for the snippet's `id` and `lang` attributes. The change is quite simple, I did however encounter some gotchas. Below is a short list of notes to explain some aspects of the change.
>> 
>>  - Both `id` and `lang` are only meaningful if they have non-empty values, so defining them with empty or missing values will cause the respective HTML attributes to be not defined.
>>  - I decided to put the logic for extracting the `id` and `lang` attributes into the already quite long `SnippetTaglet.getInlineTagOutput` method as this is where the required preparation and infrastructure are located. Both values are then passed as additional arguments to `TagletWriter.snippetTagOutput` which is not great. I think it's ok for now, but maybe if we need to pass more values along that way we should reconsider this design.
>>  - I'm not sure whether we should assume `lang=java` for internal snippets. This change currently does not, it only derives implicit `lang` attributes for external snippets based on the extension of the snippet source file.
>>  - In `SnippetTaglet.languageFromFilename()` I tried to cover what I reckoned would be the most useful languages for javadoc users, including a few major JVM languages. I'm sure I forgot some, but I hope I got at least those file extensions right.
>>  - We previously added a newline character to the snippet's `<pre>` element. I think this was done in order to make sure the element is rendered even if the snippet content is empty. This was fine with just the `<pre>` element, but with a nested `<code>` element that newline is visible in the browser. I therefore replaced the newline with `HtmlTree.EMPTY`. 
>>  - For the test, I was able to convert the existing `testIdAndLangAttributes` test to cover explicit `id` and `lang` attributes. I added a new test called `testExternalImplicitAttributes` to test derived `lang` attributes.
>
> Hannes Wallnöfer has refreshed the contents of this pull request, and previous commits have been removed. The incremental views will show differences compared to the previous content of the PR.

Marked as reviewed by prappo (Reviewer).

src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java line 277:

> 275:                         ? null
> 276:                         : stringOf(idAttr.getValue());
> 277: 

I like how clear it reads.

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

> 24: /*
> 25:  * @test
> 26:  * @bug 8266666 8275788

(Not a showstopper.) Do we have to add that bug ID? Until JDK 18 is released, maybe all changes similar to this one could be thought of as the continuation of the initial 8266666?

That said, I have to admit that I still don't know how people use the `@bug` tag. To me, `@bug` is similar to `@author` in that it doesn't provide me with more information than I could get through digging through VCS history. In fact, the VCS history is much more revealing.

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

> 138: //                    }
> 139: //                    """, "foo6", null),
> 140: 

Thanks for carefully updating the comment.

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

> 210:             classBuilder.addMembers(
> 211:                     MethodBuilder.parse("public void case%s() { }".formatted(i))
> 212:                             .setComments("A method.", s.content()));

Did you add that "A method." to increase test specificity?

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

> 242:     @Test
> 243:     public void testExternalImplicitAttributes(Path base) throws IOException {
> 244:         Path srcDir = base.resolve("src");

(Not a showstopper.) We need a test for hybrid snippets too. I will add it later.

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

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