On Fri, 31 May 2024 06:28:28 GMT, Jan Lahoda <jlah...@openjdk.org> wrote:
>> This is an attempt to add Markdown support in documentation comments to
>> JShell.
>>
>> It works by converting the Markdown text to HTML during the process of
>> resolving `{@inheritDoc}` tags.
>
> Jan Lahoda has updated the pull request with a new target base due to a merge
> or a rebase. The pull request now contains 119 commits:
>
> - Merge branch 'master' into JDK-8298405
> - Merge remote-tracking branch 'origin/JDK-8332858' into JDK-8298405
> - Fixing prefix offset.
> - Fixing handling of synthetic/rewritten links.
> - Merge remote-tracking branch 'origin/JDK-8332858' into JDK-8298405
> - 8332858: References with escapes have broken positions after they are
> transformed
> - Fixing test.
> - Merge branch 'master' into JDK-8298405
> - Merge branch '8298405.doclet-markdown-v3' into JDK-8298405
> - address review feedback
> - ... and 109 more: https://git.openjdk.org/jdk/compare/2ab8ab56...d85a2b8a
I'll approve it for what it is.
There are some minor comments about imports and tests; these can either be
fixed here in this PR, or in cleanup tasks before RDP2.
It is disappointing but not surprising to see duplication of somewhat similar
code in the `jdk.javadoc` module, especially the handling of Markdown code.
This just adds to the duplication of effort interpreting other aspects of doc
comments. I understand how we got to this point, but down the road, we should
look to share more code, translating doc comments into some intermediate
representation (such as the HTML classes currently in
`jdk.javadoc.internal.doclets.formats.html.markup`) such that we only need
minimal subsequent conversion to a near-text form., like ANSI-styled text. That
would be a good layer of abstraction to have in the `jdk.javadoc` module
anyway, and if we can expose it (internally?) to other JDK modules, to support
their use and display of doc comments, so much the better: we all win.
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 38:
> 36: import com.sun.source.doctree.DocTree;
> 37: import com.sun.source.doctree.DocTreeVisitor;
> 38: import com.sun.source.doctree.EscapeTree;
unused import
src/jdk.internal.md/share/classes/jdk/internal/markdown/MarkdownTransformer.java
line 65:
> 63: import static com.sun.tools.javac.util.Position.NOPOS;
> 64: import java.util.regex.Matcher;
> 65: import jdk.internal.org.commonmark.internal.util.Escaping;
unused (and out of order) imports
src/jdk.jshell/share/classes/jdk/internal/shellsupport/doc/JavadocHelper.java
line 103:
> 101: import jdk.internal.org.commonmark.parser.IncludeSourceSpans;
> 102: import jdk.internal.org.commonmark.parser.Parser;
> 103: import jdk.internal.org.commonmark.renderer.html.HtmlRenderer;
weirdly ordered imports
src/jdk.jshell/share/classes/jdk/internal/shellsupport/doc/JavadocHelper.java
line 518:
> 516: public Void visitLink(LinkTree node, Void p) {
> 517: if (sp.isRewrittenTree(null, dcTree, node)) {
> 518: //this links is a synthetic rewritten link,
> replace
spelling `this link`
test/langtools/jdk/internal/shellsupport/doc/JavadocHelperTest.java line 346:
> 344: "@throws java.lang.IllegalAccessException exc3\n" +
> 345: "@return value\n" +
> 346: "@since snc");
Is there a good reason to not use text blocks before/after `getSubTest`
or a single text block with an old-fashioned `.replace` for a marker string?
Comment applies to all similar instances in the test. I won't bother to note
them individually.
-------------
Marked as reviewed by jjg (Reviewer).
PR Review: https://git.openjdk.org/jdk/pull/17686#pullrequestreview-2091610752
PR Review Comment: https://git.openjdk.org/jdk/pull/17686#discussion_r1622890367
PR Review Comment: https://git.openjdk.org/jdk/pull/17686#discussion_r1622890430
PR Review Comment: https://git.openjdk.org/jdk/pull/17686#discussion_r1622895867
PR Review Comment: https://git.openjdk.org/jdk/pull/17686#discussion_r1622899268
PR Review Comment: https://git.openjdk.org/jdk/pull/17686#discussion_r1622893871
