On Fri, 29 Oct 2021 14:01:38 GMT, Hannes Wallnöfer <[email protected]> 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. > 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. If we do that, we would have to document/specify that behavior, and effectively require that people specify a `lang` attribute when the content is _not_ Java. The alternative (down the road?) would be to try and parse the content to see if it "looks like" Java. Then we have to get into accepting expression/sttement/.method/clas etc. That might interact with future wish-list features to test parse ability if the language is stated to be Java. ------------- PR: https://git.openjdk.java.net/jdk/pull/6165
