On Fri, 19 Nov 2021 10:02:26 GMT, Pavel Rappo <pra...@openjdk.org> wrote:
>> 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.
>
> You are much more knowledgeable in FileManager than I am. My thinking was
> that while it's probably not an issue with the default file manager, I
> couldn't guarantee that IllegalArgumentException is not thrown if this code
> runs with a custom file manager.
I was just reading the documented conditions for this `JavaFileManager` method
to throw `IllegalArgumentException`. That being said, one of the problems with
ILA, and potentially other unchecked exception, is that they can occur from
deep within the method that is called.
FYI, javac has a way of wrapping user-provided components so that it can better
handle unexpected exceptions from user-provided code as compared to unexpected
exceptions in javac itself. If you're interested, see`javac`
`ClientCodeWrapper`. And, to be clear, I'm *not* suggesting that for javadoc
at this time!
>> 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.
>
> I was surprised to hear that this is a convention, so much so that I wrote a
> script to survey the JDK tests in this regard. The script showed that while
> what you said is true for javadoc, it is not so true for other areas (even in
> langtools). That said, I can see the rationale for having this convention and
> will update the snippet tests accordingly.
>
> Separately. If there's a value in mass-fixing the JDK tests so that they
> follow this convention, I can create a PR to do that. Here are some numbers
> provided by the script, to assess the situation. There are 32,920
> `test/**/*.java` files. Of these, 19,190 have jtreg comments. Of these, 4,284
> (or 22%) do not follow the convention one way or another. If pursued, that
> might result in an extremely tedious-to-review PR.
Maybe the convention is not as strong as I thought. It definitely does not
warrant mass updates.
I'd be OK to review any updates needed for javadoc tests.
>> 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
>> * }
>> */
>
> What you suggested is how I had that initially. But then a test purist in me
> demanded a clear way of getting the link without interfering with the test
> logic.
>
> As for text blocks: to me, indenting a text block is a complex, subjective
> and unsettled issue. I would postpone re-indenting them for now, primarily
> because of the churn it would otherwise introduce to the dependent PRs.
>
> If neither of these issues is a showstopper, I would really like to integrate
> this PR to unlock more urgent work on the snippets feature before RDP 1,
> which is on 2021/12/09 as per the Schedule at
> https://openjdk.java.net/projects/jdk/18/. Sometime after this PR is
> integrated, we can revisit this.
OK, but the test purist in me dislikes the significant amount of inline
overhead to address this concern.
I'll pass on this for now, because I agree with unblocking the dependent PRs,
but it seems like that is a negative side-effect of dependent PRs in general.
Once we get past this, at a minimum, I would suggest moving the in-memory file
manager out of this class, and then potentially reconsider the decision that it
is good practice to do the separate run.
>> 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.
>
> I can add a comment that this test is a part of an end-to-end demonstration
> of how --snippet-path should work. This is a black-box test that assumes
> nothing about the implementation of the feature.
OK, but I still think the test is somewhat unnecessary. But now you have it,
don't delete it.
-------------
PR: https://git.openjdk.java.net/jdk/pull/6359
