Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v38]
On Thu, 22 Feb 2024 19:17:09 GMT, Jonathan Gibbons wrote: >>>I think in this place we also need to check for indented code blocks, since >>>we reduced currIndent and may have 4 or more character indentation compared >>>to the new currIndent value. >> >> This seems wrong. Surely, we should not reduce the indentation and then for >> the same line in the comment check for an extra-indented line. Or else I am >> missing something. >> >> That being said, thanks for the test case. I will investigate it. > > Update: I see and understand your test case. Update: I found that more elegant solution you hinted at, by refactoring the method from nested `if` statements to a series of checks with early returns. The test case you provided now works as expected. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1499918026
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v38]
On Thu, 22 Feb 2024 19:13:57 GMT, Jonathan Gibbons wrote: >> Thanks. I will investigate your hint to look for a more elegant solution. >> Thanks for the new test case. > >>I think in this place we also need to check for indented code blocks, since >>we reduced currIndent and may have 4 or more character indentation compared >>to the new currIndent value. > > This seems wrong. Surely, we should not reduce the indentation and then for > the same line in the comment check for an extra-indented line. Or else I am > missing something. > > That being said, thanks for the test case. I will investigate it. Update: I see and understand your test case. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1499787391
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v38]
On Thu, 22 Feb 2024 15:14:32 GMT, Jonathan Gibbons wrote:
>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>> line 1373:
>>
>>> 1371: while (indent < currIndent) {
>>> 1372: currIndent = indentStack.pop();
>>> 1373: }
>>
>> This new Markdown class looks very good!
>>
>> I think in this place we also need to check for indented code blocks, since
>> we reduced `currIndent` and may have 4 or more character indentation
>> compared to the new `currIndent` value.
>>
>> My tentative fix is to add the following code here, but maybe a more elegant
>> solution can be found by restructuring the method to first check for `indent
>> < currIndent` and then only having to check for new indented code blocks
>> once.
>>
>>
>> if (indent >= currIndent + 4 && !isParagraph(prevLineKind)) {
>> blockId++;
>> lineKind = LineKind.INDENTED_CODE_BLOCK;
>> return;
>> }
>>
>>
>> The following could be added to `TestMarkdownCodeBlocks.java` to test this
>> case:
>>
>>
>> POST_LIST_INDENT(
>> """
>> 1. list item
>>
>> second paragraph
>>
>> {@code CODE}
>> @Anno
>>
>> end""",
>> """
>>
>>
>> list item
>> second paragraph
>>
>>
>> {@code CODE}
>> @Anno
>>
>> end"""),
>
> Thanks. I will investigate your hint to look for a more elegant solution.
> Thanks for the new test case.
>I think in this place we also need to check for indented code blocks, since we
>reduced currIndent and may have 4 or more character indentation compared to
>the new currIndent value.
This seems wrong. Surely, we should not reduce the indentation and then for the
same line in the comment check for an extra-indented line. Or else I am missing
something.
That being said, thanks for the test case. I will investigate it.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1499779864
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v38]
On Thu, 22 Feb 2024 14:53:11 GMT, Hannes Wallnöfer wrote:
>> Jonathan Gibbons has updated the pull request incrementally with two
>> additional commits since the last revision:
>>
>> - update DocCommentParser and tests to improve handling of code blocks and
>> code spans in Markdown documentation comments
>> - fix indentation, for consistency
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
> line 1373:
>
>> 1371: while (indent < currIndent) {
>> 1372: currIndent = indentStack.pop();
>> 1373: }
>
> This new Markdown class looks very good!
>
> I think in this place we also need to check for indented code blocks, since
> we reduced `currIndent` and may have 4 or more character indentation compared
> to the new `currIndent` value.
>
> My tentative fix is to add the following code here, but maybe a more elegant
> solution can be found by restructuring the method to first check for `indent
> < currIndent` and then only having to check for new indented code blocks once.
>
>
> if (indent >= currIndent + 4 && !isParagraph(prevLineKind)) {
> blockId++;
> lineKind = LineKind.INDENTED_CODE_BLOCK;
> return;
> }
>
>
> The following could be added to `TestMarkdownCodeBlocks.java` to test this
> case:
>
>
> POST_LIST_INDENT(
> """
> 1. list item
>
> second paragraph
>
> {@code CODE}
> @Anno
>
> end""",
> """
>
>
> list item
> second paragraph
>
>
> {@code CODE}
> @Anno
>
> end"""),
Thanks. I will investigate your hint to look for a more elegant solution.
Thanks for the new test case.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1499408829
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v38]
On Thu, 22 Feb 2024 00:17:29 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 two
> additional commits since the last revision:
>
> - update DocCommentParser and tests to improve handling of code blocks and
> code spans in Markdown documentation comments
> - fix indentation, for consistency
src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
line 1373:
> 1371: while (indent < currIndent) {
> 1372: currIndent = indentStack.pop();
> 1373: }
This new Markdown class looks very good!
I think in this place we also need to check for indented code blocks, since we
reduced `currIndent` and may have 4 or more character indentation compared to
the new `currIndent` value.
My tentative fix is to add the following code here, but maybe there's a more
elegant solution by restructuring the method to first check for `indent <
currIndent` and then for new indented code blocks.
if (indent >= currIndent + 4 && !isParagraph(prevLineKind)) {
blockId++;
lineKind = LineKind.INDENTED_CODE_BLOCK;
return;
}
The following could be added to `TestMarkdownCodeBlocks.java` to test this case:
POST_LIST_INDENT(
"""
1. list item
second paragraph
{@code CODE}
@Anno
end""",
"""
list item
second paragraph
{@code CODE}
@Anno
end"""),
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1499375866
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v38]
> 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 two additional commits since the last revision: - update DocCommentParser and tests to improve handling of code blocks and code spans in Markdown documentation comments - fix indentation, for consistency - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/25921fd0..5fc9c84a Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk=16388=37 - incr: https://webrevs.openjdk.org/?repo=jdk=16388=36-37 Stats: 1240 lines in 4 files changed: 918 ins; 275 del; 47 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
