On Fri, 12 Nov 2021 16:28:09 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> This change is mostly about tests. I say "mostly" because while increasing
>> code coverage, which was the primary goal of this exercise, I uncovered a
>> few non-critical bugs and fixed them in-place. The net effect of the change
>> boils down to these code coverage statistics.
>>
>> before
>> ------
>>
>> %method %block %branch %line
>> jdk.javadoc.internal.doclets.toolkit.taglets.SnippetTaglet
>> 75%(12/16)
>> 87%(109/124)
>> 88%(99/112)
>> 85%(140/164)
>>
>> #classes %method %block %branch %line
>> jdk.javadoc.internal.doclets.toolkit.taglets.snippet
>> 70%(80/114)
>> 76%(310/407)
>> 65%(178/273)
>> 81%(413/508)
>>
>> after
>> -----
>>
>> %method %block %branch %line
>> jdk.javadoc.internal.doclets.toolkit.taglets.SnippetTaglet
>> 100%(17/17)
>> 95%(120/126)
>> 93%(103/110)
>> 97%(163/168)
>>
>> %method %block %branch %line
>> jdk.javadoc.internal.doclets.toolkit.taglets.snippet
>> 83%(94/112)
>> 85%(348/405)
>> 73%(202/273)
>> 91%(463/505)
>
> Pavel Rappo has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Test one more corner case example
In general, great work. Most of my comments are about style, especially
unindented text blocks.
The one aspect that I think deserves attention is the code for the `@link`
feature and the use of in-memory file objects and file managers. While not
wrong, it seems overkill and unnecessary, since I think you could embed the
reference tag and the markup tag in the same file, without going all the way to
the separate run of javadoc. Given that it's not wrong, and that there are
dependent reviews blocked by this, I'll approve it if you want, but would like
to understand if there are reasons to do it as you have.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java
line 94:
> 92: }
> 93:
> 94: private static final class BadSnippetException extends Exception {
General comment: I like the general use of `BadSnippetException`. Very nice.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/SnippetTaglet.java
line 202:
> 200: fileObject =
> fileManager.getFileForInput(Location.SNIPPET_PATH, "", v);
> 201: }
> 202: } catch (IOException | IllegalArgumentException e) { //
> TODO: test this when JDK-8276892 is integrated
FYI: The documented conditions of `IllegalArgumentException` do not arise in
this context, although it's not wrong to catch the exception.
src/jdk.javadoc/share/classes/jdk/javadoc/internal/doclets/toolkit/taglets/snippet/Replace.java
line 79:
> 77: replacements.add(new Replacement(start, end, s));
> 78: }
> 79: // there's no need to call matcher.appendTail(b)
(minor) maybe add a brief reason to the comment
test/langtools/jdk/javadoc/doclet/testSnippetTag/SnippetTester.java line 90:
> 88: }
> 89:
> 90: // TODO: perhaps this method could be added to JavadocTester
There should be an equivalent feature there now
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 71:
> 69: * @build javadoc.tester.* toolbox.ToolBox toolbox.ModuleBuilder
> builder.ClassBuilder
> 70: * @run main TestSnippetMarkup
> 71: */
The standard convention is to put the jtreg comment right after the legal
header, before the imports.
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line 74:
> 72: public class TestSnippetMarkup extends SnippetTester {
> 73:
> 74: private final ToolBox tb = new ToolBox();
You might want to move this into `SnippetTester`
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line
185:
> 183: link(First) link(line)
> 184: Second line
> 185: """, "link\\((.+?)\\)", r -> link(true,
> "java.lang.Object#Object", r.group(1)))
clever
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line
191:
> 189:
> 190: @Test
> 191: public void testCornerCases(Path base) throws Exception {
nice corner cases
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line
230:
> 228: * next line.
> 229: */
> 230: // @Test
(minor) maybe add `// TODO`
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line
309:
> 307: ^""".formatted(badFile),
> 308: """
> 309: %s:1: error: snippet markup: tag refers to non-existent lines
The zero-indent on these lines makes them slightly less convenient to read in
context.
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line
388:
> 386: // generate documentation at low cost and do not interfere with
> the
> 387: // calling test state; for that, do not create file trees, do
> not write
> 388: // to std out/err, and generally try to keep everything in memory
I understand what you're doing here, but I'm surprised you need to go to the
trouble of a separate run of javadoc to get the link output. Can you not put
the link tag and the link markup tag in the same source, and then extract the
two `<a>` from the generated file? Maybe put distinguishing characters around
each instance, to make locating them easier. For example:
/**
* First sentence. TAG {@link Object} TAG.
* {@snippet :
* MARKUP Object MARKUP // @link substring=Object target=Object
* }
*/
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line
395:
> 393: """.formatted(targetReference, content);
> 394:
> 395: JavaFileObject src = new JavaFileObject() {
Check out `SimpleJavaFileObject` for use as a super type.
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetMarkup.java line
456:
> 454:
> 455: // FileManager has to be StandardJavaFileManager; JavaDoc is
> adamant about it
> 456: class InMemoryFileManager extends ToolBox.MemoryFileManager
(Future?) this could be promoted to a top level class, or merged into the
toolbox class.
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetPathOption.java
line 43:
> 41: * @build javadoc.tester.* toolbox.ToolBox toolbox.ModuleBuilder
> builder.ClassBuilder
> 42: * @run main TestSnippetPathOption
> 43: */
before imports?
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetPathOption.java
line 46:
> 44: public class TestSnippetPathOption extends SnippetTester {
> 45:
> 46: private final ToolBox tb = new ToolBox();
(minor) move to SnippetTester
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetPathOption.java
line 131:
> 129: .map(Path::toAbsolutePath)
> 130: .map(Path::toString)
> 131: .collect(Collectors.joining(File.pathSeparator));
(minor) This sequence is showing up enough to warrant being a method somewhere.
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetPathOption.java
line 168:
> 166: */
> 167: @Test
> 168: public void testSearchPath(Path base) throws Exception {
Not wrong, but arguably somewhat superfluous. This is close to being a file
manager test, not a snippet test.
test/langtools/jdk/javadoc/doclet/testSnippetTag/TestSnippetTag.java line 1193:
> 1191: : error: snippet markup: missing attribute "region"
> 1192: // @start
> 1193: ^""");
(style) why not indent the text?
-------------
PR: https://git.openjdk.java.net/jdk/pull/6359
