Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]

2024-02-15 Thread Pavel Rappo
On Thu, 15 Feb 2024 19:39:07 GMT, Pavel Rappo  wrote:

>> whitespace is handled separately, on line 280 (`readIndent`) and285 (`: 
>> (indent <= 3) ? peekLineKind()`)
>
> Correct, but I believe the ordered list marker should be like this:
> 
> ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)] .*"))
>  ^
> 
> Note extra whitespace character. I think we should really add this to our 
> tests, since lists are foundational Markdown structures, probably right after 
> italics and bold.

Then we should add `[ \t]` to both constants, right:

BULLETED_LIST_ITEM(Pattern.compile("[-+*][ \t].*"))
ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)][ \t].*"))

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491564839


Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]

2024-02-15 Thread Jonathan Gibbons
On Thu, 15 Feb 2024 19:39:07 GMT, Pavel Rappo  wrote:

>> whitespace is handled separately, on line 280 (`readIndent`) and285 (`: 
>> (indent <= 3) ? peekLineKind()`)
>
> Correct, but I believe the ordered list marker should be like this:
> 
> ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)] .*"))
>  ^
> 
> Note extra whitespace character. I think we should really add this to our 
> tests, since lists are foundational Markdown structures, probably right after 
> italics and bold.

My recollection is that the space is required.   I will double check.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491552312


Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]

2024-02-15 Thread Pavel Rappo
On Thu, 15 Feb 2024 19:20:23 GMT, Jonathan Gibbons  wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>>  line 1401:
>> 
>>> 1399:  */
>>> 1400: enum LineKind {
>>> 1401: BLANK(Pattern.compile("[ \t]*")),
>> 
>> `BLANK` is a pseudo kind, because it is set manually, but never returned 
>> from `peekLine()`. I wonder if we can change it somehow.
>
> Even if it is set manually, it is still appropriate to have it as a member in 
> the `LineKind` enum class.

Not that it invalidates your reply; clarification: I meant `peekLineKind()`, 
not `peekLine()`.

>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>>  line 1433:
>> 
>>> 1431:  * @see >> href="https://spec.commonmark.org/0.30/#list-items";>List items
>>> 1432:  */
>>> 1433: BULLETED_LIST_ITEM(Pattern.compile("[-+*] .*")),
>> 
>> This comment is about `BULLETED_LIST_ITEM` and `ORDERED_LIST_ITEM` 
>> constants. I know that we don't need to be very precise, but perhaps in this 
>> case we should. While the CommonMark spec is a vague on that, from my 
>> experiments with [dingus](https://spec.commonmark.org/dingus/), it appears 
>> that a list marker can be preceded and followed by some number of whitespace 
>> characters.
>
> whitespace is handled separately, on line 280 (`readIndent`) and285 (`: 
> (indent <= 3) ? peekLineKind()`)

Correct, but I believe the ordered list marker should be like this:

ORDERED_LIST_ITEM(Pattern.compile("[0-9]{1,9}[.)] .*"))
 ^

Note extra whitespace character. I think we should really add this to our 
tests, since lists are foundational Markdown structures, probably right after 
italics and bold.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491550784
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491545609


Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]

2024-02-15 Thread Jonathan Gibbons
On Thu, 15 Feb 2024 19:27:12 GMT, Jonathan Gibbons  wrote:

>> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>>  line 422:
>> 
>>> 420: defaultContentCharacter();
>>> 421: }
>>> 422: }
>> 
>> Is it to pass `` through to Markdown, to allow it to deal with Markdown 
>> escapes?
>
> It is more about supporting the sequence `` ` `` so that the backtick is 
> treated as a literal character and not the beginning of a code span.   This 
> is the "backstop" preferred way to ensure that a single backtick is treated 
> literally, without relying on detected that it is unbalanced when the end of 
> the current block is reached.  The alternative workaround would be a single 
> backtick enclosed in multiple backticks, such as this ``` `` ` `` ```. (I 
> leave you to figure out what I actually typed there!!!)

You might also need to use `` ` `` when there are two literal backticks in a 
sentence.

After the first backtick (`), another backtick (`) can be used to delimit a 
code span.

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491528153


Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]

2024-02-15 Thread Jonathan Gibbons
On Thu, 15 Feb 2024 17:17:46 GMT, Pavel Rappo  wrote:

>> Jonathan Gibbons has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains 44 commits:
>> 
>>  - fill in `visitRawText` in `CommentHelper.getTags` visitor
>>  - fixes for the "New API" page
>>  - change "standard" to "traditional" when referring to a comment
>>  - Merge remote-tracking branch 'upstream/master' into 
>> 8298405.doclet-markdown-v3
>>  - Merge remote-tracking branch 'upstream/master' into 
>> 8298405.doclet-markdown-v3
>>  - improve support for DocCommentParser.LineKind
>>  - Merge remote-tracking branch 'upstream/master' into 
>> 8298405.doclet-markdown-v3 # Please enter a commit message to explain why 
>> this merge is necessary, # especially if it merges an updated upstream into 
>> a topic branch. # # Lines starting with '#' will be ignored, and an empty 
>> message aborts # the
>>commit.
>>  - update copyright year on test
>>  - refactor recent new test case in TestMarkdown.java
>>  - address review feedback
>>  - ... and 34 more: https://git.openjdk.org/jdk/compare/8765b176...2801c2e1
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>  line 422:
> 
>> 420: defaultContentCharacter();
>> 421: }
>> 422: }
> 
> Is it to pass `` through to Markdown, to allow it to deal with Markdown 
> escapes?

It is more about supporting the sequence `` ` `` so that the backtick is 
treated as a literal character and not the beginning of a code span.   This is 
the "backstop" preferred way to ensure that a single backtick is treated 
literally, without relying on detected that it is unbalanced when the end of 
the current block is reached.  The alternative workaround would be a single 
backtick enclosed in multiple backticks, such as this ``` `` ` `` ```. (I leave 
you to figure out what I actually typed there!!!)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491519707


Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]

2024-02-15 Thread Jonathan Gibbons
On Thu, 15 Feb 2024 17:03:09 GMT, Pavel Rappo  wrote:

>> Jonathan Gibbons has updated the pull request with a new target base due to 
>> a merge or a rebase. The pull request now contains 44 commits:
>> 
>>  - fill in `visitRawText` in `CommentHelper.getTags` visitor
>>  - fixes for the "New API" page
>>  - change "standard" to "traditional" when referring to a comment
>>  - Merge remote-tracking branch 'upstream/master' into 
>> 8298405.doclet-markdown-v3
>>  - Merge remote-tracking branch 'upstream/master' into 
>> 8298405.doclet-markdown-v3
>>  - improve support for DocCommentParser.LineKind
>>  - Merge remote-tracking branch 'upstream/master' into 
>> 8298405.doclet-markdown-v3 # Please enter a commit message to explain why 
>> this merge is necessary, # especially if it merges an updated upstream into 
>> a topic branch. # # Lines starting with '#' will be ignored, and an empty 
>> message aborts # the
>>commit.
>>  - update copyright year on test
>>  - refactor recent new test case in TestMarkdown.java
>>  - address review feedback
>>  - ... and 34 more: https://git.openjdk.org/jdk/compare/8765b176...2801c2e1
>
> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>  line 1401:
> 
>> 1399:  */
>> 1400: enum LineKind {
>> 1401: BLANK(Pattern.compile("[ \t]*")),
> 
> `BLANK` is a pseudo kind, because it is set manually, but never returned from 
> `peekLine()`. I wonder if we can change it somehow.

Even if it is set manually, it is still appropriate to have it as a member in 
the `LineKind` enum class.

> src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java
>  line 1433:
> 
>> 1431:  * @see > href="https://spec.commonmark.org/0.30/#list-items";>List items
>> 1432:  */
>> 1433: BULLETED_LIST_ITEM(Pattern.compile("[-+*] .*")),
> 
> This comment is about `BULLETED_LIST_ITEM` and `ORDERED_LIST_ITEM` constants. 
> I know that we don't need to be very precise, but perhaps in this case we 
> should. While the CommonMark spec is a vague on that, from my experiments 
> with [dingus](https://spec.commonmark.org/dingus/), it appears that a list 
> marker can be preceded and followed by some number of whitespace characters.

whitespace is handled separately, on line 280 (`readIndent`) and285 (`: (indent 
<= 3) ? peekLineKind()`)

-

PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491508303
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491512611


Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]

2024-02-15 Thread Pavel Rappo
On Thu, 15 Feb 2024 00:30:25 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 with a new target base due to a 
> merge or a rebase. The pull request now contains 44 commits:
> 
>  - fill in `visitRawText` in `CommentHelper.getTags` visitor
>  - fixes for the "New API" page
>  - change "standard" to "traditional" when referring to a comment
>  - Merge remote-tracking branch 'upstream/master' into 
> 8298405.doclet-markdown-v3
>  - Merge remote-tracking branch 'upstream/master' into 
> 8298405.doclet-markdown-v3
>  - improve support for DocCommentParser.LineKind
>  - Merge remote-tracking branch 'upstream/master' into 
> 8298405.doclet-markdown-v3 # Please enter a commit message to explain why 
> this merge is necessary, # especially if it merges an updated upstream into a 
> topic branch. # # Lines starting with '#' will be ignored, and an empty 
> message aborts # the
>commit.
>  - update copyright year on test
>  - refactor recent new test case in TestMarkdown.java
>  - address review feedback
>  - ... and 34 more: https://git.openjdk.org/jdk/compare/8765b176...2801c2e1

I'm again looking `LineKind`. This time because of 9eaf84e5dd6.

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java 
line 422:

> 420: defaultContentCharacter();
> 421: }
> 422: }

Is it to pass `` through to Markdown, to allow it to deal with Markdown escapes?

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java 
line 1401:

> 1399:  */
> 1400: enum LineKind {
> 1401: BLANK(Pattern.compile("[ \t]*")),

`BLANK` is a pseudo kind, because it is set manually, but never returned from 
`peekLine()`. I wonder if we can change it somehow.

src/jdk.compiler/share/classes/com/sun/tools/javac/parser/DocCommentParser.java 
line 1433:

> 1431:  * @see  href="https://spec.commonmark.org/0.30/#list-items";>List items
> 1432:  */
> 1433: BULLETED_LIST_ITEM(Pattern.compile("[-+*] .*")),

This comment is about `BULLETED_LIST_ITEM` and `ORDERED_LIST_ITEM` constants. I 
know that we don't need to be very precise, but perhaps in this case we should. 
While the CommonMark spec is a vague on that, from my experiments with 
[dingus](https://spec.commonmark.org/dingus/), it appears that a list marker 
can be preceded and followed by some number of whitespace characters.

-

PR Review: https://git.openjdk.org/jdk/pull/16388#pullrequestreview-1883374712
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491362821
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491344667
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1491354450


Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v32]

2024-02-14 Thread Jonathan Gibbons
> 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 with a new target base due to a 
merge or a rebase. The pull request now contains 44 commits:

 - fill in `visitRawText` in `CommentHelper.getTags` visitor
 - fixes for the "New API" page
 - change "standard" to "traditional" when referring to a comment
 - Merge remote-tracking branch 'upstream/master' into 
8298405.doclet-markdown-v3
 - Merge remote-tracking branch 'upstream/master' into 
8298405.doclet-markdown-v3
 - improve support for DocCommentParser.LineKind
 - Merge remote-tracking branch 'upstream/master' into 
8298405.doclet-markdown-v3 # Please enter a commit message to explain why this 
merge is necessary, # especially if it merges an updated upstream into a topic 
branch. # # Lines starting with '#' will be ignored, and an empty message 
aborts # the
   commit.
 - update copyright year on test
 - refactor recent new test case in TestMarkdown.java
 - address review feedback
 - ... and 34 more: https://git.openjdk.org/jdk/compare/8765b176...2801c2e1

-

Changes: https://git.openjdk.org/jdk/pull/16388/files
 Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=31
  Stats: 22094 lines in 197 files changed: 21411 ins; 286 del; 397 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