Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v28]
On Thu, 8 Feb 2024 15:38:26 GMT, Pavel Rappo wrote: >> test/langtools/jdk/javadoc/tool/sampleapi/lib/sampleapi/generator/ModuleGenerator.java >> line 49: >> >>> 47: import sampleapi.util.PoorDocCommentTable; >>> 48: >>> 49: import static >>> com.sun.tools.javac.parser.Tokens.Comment.CommentStyle.JAVADOC; >> >> To clarify, the change to this file is that you removed two unused imports, >> right? > > Ping. Maybe if it is not essential, we could remove that change, to keep PR > more focused. Okay I can see that you tried to revert the change to that file and published a [PR] to optimise imports separately. Sadly, the "revert" introduced even more churn. Here's my proposal, which might not be elegant but does the job: git checkout 8298405.doclet-markdown-v3 git diff -R head --not upstream/master -- test/langtools/jdk/javadoc/tool/sampleapi/lib/sampleapi/generator/ModuleGenerator.java | git apply git commit -am "Revert all changes to ModuleGenerator.java" If you have any issues with that snippet, ping me out of band; I hope the intent of the snippet is clear though. [PR]: https://github.com/openjdk/jdk/pull/17782 - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484477033
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v28]
On Thu, 8 Feb 2024 21:46:34 GMT, Jonathan Gibbons wrote:
>> I believe this is substantially covered in line 226-227. See the third call
>> to `test` in the following group of lines:
>>
>>
>> for (String src : sources) {
>> test(src);
>> test(src.replaceAll("@Deprecated", "/** @deprecated */"));
>> test(src.replaceAll("deprecated", "notDeprecated2") //
>> change class name
>> .replaceAll("@Deprecated", "/// @deprecated\n"));
>> }
>>
>>
>> * The first call, `test(src)`, runs all the test cases, using the
>> `@Deprecated` annotation by default. In these test cases, the name of the
>> element encodes whether it is expected that the element is deprecated or not.
>>
>> * The second call, `test(src.replaceAll("deprecated", "notDeprecated2")`,
>> runs the test cases again, after changing the annotation to a traditional
>> (`/** ...*/`) comment containing the `@deprecated` tag. This is a
>> long-standing call, and tests the legacy behavior of `@deprecated` appearing
>> in a traditional doc comment. Effectively, it tests whether a `/**
>> @deprecated */` comment has equivalent behavior to a `@Deprecated` comment.
>>
>> * The third call is new to this PR and the functionality to support Markdown
>> comments. It makes two changes to the source for the test cases, before
>> running them:
>>1. as in the previous test, the annotations are replaced by a comment
>> containing `@deprecated` -- except that this time, the comment is a
>> new-style `/// ... ` comment instead of a traditional `/** ... */` comment,
>> and ...
>>2. because we do not expect `/// @deprecated` to indicate deprecation, we
>> need to change the expected result for each test case, which we do by
>> changing the element name for the test case. The change is the first call
>> to replace to avoid false positives after changing the doc comment. The
>> change uses a new name, `notDeprecated2`, in which `notDeprecated` encodes
>> the expected deprecation status, and the `2` is to differentiate the
>> declarations from other declarations in the case case that already use the
>> name `notDeprecated`.
>
> While the underlying mechanism in javac for indicating deprecation is the
> same for all, I accept this is primarily a test for generated class files. I
> can add a javadoc test for basic behavior of `@deprecated` in Markdown
> comments.
Thanks for confirming that there's a test to check that `/// @deprecated` is a
no-op and for patiently explaining how that test works.
I confirm that
test/langtools/tools/javac/classfiles/attributes/deprecated/DeprecatedTest.java
starts failing if I introduce the following change:
---
a/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java
+++
b/src/jdk.compiler/share/classes/com/sun/tools/javac/parser/JavaTokenizer.java
@@ -1638,7 +1638,7 @@ protected void scanDocComment() {
? trimJavadocLineComment(line, indent)
: trimJavadocComment(line);
-if (cs == CommentStyle.JAVADOC_BLOCK) {
+//if (cs == CommentStyle.JAVADOC_BLOCK) {
// If standalone @deprecated tag
int pos = line.position();
line.skipWhitespace();
@@ -1652,7 +1652,7 @@ protected void scanDocComment() {
}
line.reset(pos);
-}
+//}
putLine(line);
}
I can also see that you have introduced a dedicated
test/langtools/jdk/javadoc/doclet/testMarkdown/TestMarkdown.java#testDeprecated
which also starts failing with that change of mine.
Please update the copyright year in
test/langtools/jdk/javadoc/doclet/testMarkdown/TestMarkdown.java.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484410402
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v28]
On Wed, 15 Nov 2023 00:37:06 GMT, Jonathan Gibbons wrote:
>> test/langtools/jdk/javadoc/tool/testTransformer/TestTransformer.java line 96:
>>
>>> 94: var sl =
>>> ServiceLoader.load(DocTrees.DocCommentTreeTransformer.class);
>>> 95: return StreamSupport.stream(sl.spliterator(), false)
>>> 96: .filter(p -> p.name().equals(name))
>>
>> ServiceLoader specification suggests another way of streaming:
>> Suggestion:
>>
>> return sl.stream()
>> .map(ServiceLoader.Provider::get)
>> .filter(t -> t.name().equals(name))
>>
>> Would it be the same and more readable?
>
> Hmm. It's longer, but arguably simpler to comprehend than using a
> spliterator. I've changed the code.
There's now a leftover `import java.util.stream.StreamSupport;` in that file.
>> test/langtools/tools/javac/doctree/MDPrinter.java line 67:
>>
>>> 65: * Conceptually based on javac's {@code DPrinter}.
>>> 66: */
>>> 67: public class MDPrinter {
>>
>> While DPrinter is used, MDPrinter isn't. (At least, I could't find any
>> usages of it.) If you feel like MDPrinter is important, we should at least
>> add a compile test for it, to protect it from bitrot.
>
> 👍
MDPrinter.java is now a test that compiles itself; thanks.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484541756
PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484196535
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v28]
On Thu, 8 Feb 2024 15:35:58 GMT, Pavel Rappo wrote: >> Please update the DocComment printout in that commented out test: the actual >> content is different. It would be nice if the test were passing at least at >> the moment of its initial commit. >> >> Here's what I see locally: >> >> >> Expect: >> DocComment[DOC_COMMENT, pos:0 >> firstSentence: 1 >> Summary[SUMMARY, pos:4 >> summary: 1 >> Erroneous[ERRONEOUS, pos:14, prefPos:37 >> code: compiler.err.dc.unterminated.inline.tag >> body: abc_`|_def}|_rest_`more` >> ] >> ] >> body: empty >> block tags: empty >> ] >> >> Found: >> DocComment[DOC_COMMENT, pos:0 >> firstSentence: 1 >> Summary[SUMMARY, pos:1 >> summary: 1 >> Erroneous[ERRONEOUS, pos:11, prefPos:32 >> code: compiler.err.dc.unterminated.inline.tag >> body: abc_`|def}|rest_`more` >> ] >> ] >> body: empty >> block tags: empty >> ] > > Ping. Thanks for fixing the test and clarifying its comment. - PR Review Comment: https://git.openjdk.org/jdk/pull/16388#discussion_r1484174598
Re: RFR: JDK-8298405: Support Markdown in Documentation Comments [v28]
> 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: address review feedback - Changes: - all: https://git.openjdk.org/jdk/pull/16388/files - new: https://git.openjdk.org/jdk/pull/16388/files/3b1350b2..91588bc3 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=27 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=16388&range=26-27 Stats: 23 lines in 2 files changed: 4 ins; 18 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
