Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v43]
On Wed, 28 Feb 2024 17:12:04 GMT, Jonathan Gibbons wrote: >> src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java >> line 1601: >> >>> 1599: : eKind != ElementKind.OTHER ? 1 // module, >>> package, class, interface >>> 1600: : 0; // doc file >>> 1601: return "h" + Math.min(heading.getLevel() + offset, 6); >> >> I really like that we adapt the heading level to the current context, but I >> notice that the code kind of expects `h1` headings to be used everywhere, >> and "punishes" use of contextually correct headings by generating smaller >> headings. Maybe it would be more educational to only adjust the level if it >> needs adjusting? > > Setext headings only come in "level 1" and "level 2" flavors. > And, at the time the renderer sees the AST, the difference between ATX and > setext headings is erased. They're just "headings". > > I also think it is better to have a simple rule than an "adaptive" rule. If > you start doing a more complex rule, you have to consider the effect on > subheadings as well. I suspected it was about the limited range of Setext headings. Yesterday my proposal was intentionally vague, but thinking about this again I think we should actually do the simplest and least intrusive thing possible: // minLevel is 4 for members, 2 for page-level elements, 1 for doc files "h" + Math.max(heading.getLevel(), minLevel); This arguably generates the correct heading for most simple use cases. What it doesn't do is to translate whole hierarchies of headings, but I would argue that few people people need this and those who do should figure out the rules and use the correct ATX headings. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1507439797
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v43]
On Wed, 28 Feb 2024 15:54:38 GMT, Hannes Wallnöfer wrote: >> Jonathan Gibbons has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Refactor most of TestMarkdown.java into separate tests, grouped by >> functionality > > src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java > line 1601: > >> 1599: : eKind != ElementKind.OTHER ? 1 // module, >> package, class, interface >> 1600: : 0; // doc file >> 1601: return "h" + Math.min(heading.getLevel() + offset, 6); > > I really like that we adapt the heading level to the current context, but I > notice that the code kind of expects `h1` headings to be used everywhere, and > "punishes" use of contextually correct headings by generating smaller > headings. Maybe it would be more educational to only adjust the level if it > needs adjusting? Setext headings only come in "level 1" and "level 2" flavors. And, at the time the renderer sees the AST, the difference between ATX and setext headings is erased. They're just "headings". - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1506307115
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v43]
On Fri, 23 Feb 2024 22:27:43 GMT, Jonathan Gibbons wrote: >> Please review a patch to add support for Markdown syntax in documentation >> comments, as described in the associated JEP. >> >> Notable features: >> >> * support for `///` documentation comments in `JavaTokenizer` >> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` >> library >> * updates to `DocCommentParser` to treat `///` comments as Markdown >> * updates to the standard doclet to render Markdown comments in HTML > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Refactor most of TestMarkdown.java into separate tests, grouped by > functionality src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 1601: > 1599: : eKind != ElementKind.OTHER ? 1 // module, > package, class, interface > 1600: : 0; // doc file > 1601: return "h" + Math.min(heading.getLevel() + offset, 6); I really like that we adapt the heading level to the current context, but I notice that the code kind of expects `h1` headings to be used everywhere, and "punishes" use of contextually correct headings by generating smaller headings. Maybe it would be more educational to only adjust the level if it needs adjusting? - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1506190847
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v43]
On Fri, 23 Feb 2024 22:27:43 GMT, Jonathan Gibbons wrote: >> Please review a patch to add support for Markdown syntax in documentation >> comments, as described in the associated JEP. >> >> Notable features: >> >> * support for `///` documentation comments in `JavaTokenizer` >> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` >> library >> * updates to `DocCommentParser` to treat `///` comments as Markdown >> * updates to the standard doclet to render Markdown comments in HTML > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Refactor most of TestMarkdown.java into separate tests, grouped by > functionality src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 1733: > 1731: return false; > 1732: } > 1733: The two methods below and some other methods in this class have too much indentation. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1506145051
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v43]
On Fri, 23 Feb 2024 22:27:43 GMT, Jonathan Gibbons wrote: >> Please review a patch to add support for Markdown syntax in documentation >> comments, as described in the associated JEP. >> >> Notable features: >> >> * support for `///` documentation comments in `JavaTokenizer` >> * new module `jdk.internal.md` -- a private copy of the `commonmark-java` >> library >> * updates to `DocCommentParser` to treat `///` comments as Markdown >> * updates to the standard doclet to render Markdown comments in HTML > > Jonathan Gibbons has updated the pull request incrementally with one > additional commit since the last revision: > > Refactor most of TestMarkdown.java into separate tests, grouped by > functionality src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/formats/html/HtmlDocletWriter.java line 1380: > 1378: > 1379: var useMarkdown = trees.stream().anyMatch(t -> t.getKind() == > Kind.MARKDOWN); > 1380: var markdownHandler = useMarkdown ? new > MarkdownHandler(element) : null; The `MarkdownHandler` and `HeadingNodeRenderer` classes must become aware of the current `TagletWriter.Context`. That's because headings and other block tags should only be generated if `TagletWriter.Context.isFirstSentence` is `false`. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1506084275
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v43]
> Please review a patch to add support for Markdown syntax in documentation > comments, as described in the associated JEP. > > Notable features: > > * support for `///` documentation comments in `JavaTokenizer` > * new module `jdk.internal.md` -- a private copy of the `commonmark-java` > library > * updates to `DocCommentParser` to treat `///` comments as Markdown > * updates to the standard doclet to render Markdown comments in HTML Jonathan Gibbons has updated the pull request incrementally with one additional commit since the last revision: Refactor most of TestMarkdown.java into separate tests, grouped by functionality - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/d460ee33..398f93fc Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16388=42 - incr: https://webrevs.openjdk.org/?repo=jdk=16388=41-42 Stats: 2001 lines in 9 files changed: 1143 ins; 857 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/16388.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/16388/head:pull/16388 PR: https://git.openjdk.org/jdk/pull/16388
