Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8266666: Implementation for snippets #4795

Closed
wants to merge 40 commits into from
Closed

Conversation

pavelrappo
Copy link
Member

@pavelrappo pavelrappo commented Jul 15, 2021

This PR implements JEP 413 "Code Snippets in Java API Documentation", which hasn't been yet proposed to target JDK 18. The PR starts as a squashed merge of the https://github.com/openjdk/jdk-sandbox/tree/jdk.javadoc/snippets branch.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

Reviewers

Contributors

  • Jonathan Gibbons <jjg@openjdk.org>
  • Hannes Wallnöfer <hannesw@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4795/head:pull/4795
$ git checkout pull/4795

Update a local copy of the PR:
$ git checkout pull/4795
$ git pull https://git.openjdk.java.net/jdk pull/4795/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4795

View PR using the GUI difftool:
$ git pr show -t 4795

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4795.diff

pavelrappo added 3 commits Jul 15, 2021
This undoes use of the feature in JDK, but leaves the implementation.
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 15, 2021

👋 Welcome back prappo! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jul 15, 2021
@pavelrappo
Copy link
Member Author

@pavelrappo pavelrappo commented Jul 15, 2021

/contributor add jjg
/contributor add hannesw

@openjdk
Copy link

@openjdk openjdk bot commented Jul 15, 2021

@pavelrappo
Contributor Jonathan Gibbons <jjg@openjdk.org> successfully added.

@pavelrappo
Copy link
Member Author

@pavelrappo pavelrappo commented Jul 15, 2021

/contributor add hannesw

@openjdk
Copy link

@openjdk openjdk bot commented Jul 15, 2021

@pavelrappo
Contributor Hannes Wallnöfer <hannesw@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 15, 2021

@pavelrappo
Contributor Hannes Wallnöfer <hannesw@openjdk.org> successfully added.

@openjdk
Copy link

@openjdk openjdk bot commented Jul 15, 2021

@pavelrappo The following labels will be automatically applied to this pull request:

  • compiler
  • javadoc

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added javadoc compiler labels Jul 15, 2021
@jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented Jul 23, 2021

/csr

@openjdk openjdk bot added the csr label Jul 23, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 23, 2021

@jonathan-gibbons this pull request will not be integrated until the CSR request JDK-8266669 for issue JDK-8266666 has been approved.

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

The overall structure looks generally good. Nice work.

That being said, I note the following ...

  • All new top-level classes should have a reasonable documention comment, including where appropriate the standard boilerplate (i.e. not on public API classes and interfaces)

    * <p><b>This is NOT part of any supported API.
    * If you write code that depends on this, you do so at your own risk.
    * This code and its internal interfaces are subject to change or
    * deletion without notice.

  • All new non-private methods should have a reasonable documentation comment

  • There's more than a typical number of chatty comments and/or fixme comments and/or code that needs to be uncommented. I'll do a separate review pass to note some of those examples.

  • Please work with @hns to determine the proposed CSS style(s) to be used, and not just the interim placeholder sandybrown. The color should be compatible with the existing color palette. Ideally, at the same time, in either this changeset or a sibling changeset, we should change the style of a default <pre>{code} block to be similar with respect to the basic display characteristics.

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

More feedback;

  1. It seems reasonable to introduce the ...taglets.snippets package. It seems excessive to have three sub packages within that, for actions, parser and text, especially when the last contains just a single class.

  2. At some point soonish, we will need API somewhere to get the content of the snippet. This is necessary to fulfill the goal in the JEP to support validation of snippet content. Without any such API, validation code would have to duplicate at least some of the code to handle snippets. This API should probably not be in com.sun.source and should probably be in the jdk.javadoc module, perhaps on the existing StandardDoclet class. One way to do that would be to introduce a utility interface to provide access to support for taglets, and then have a method on StandardDoclet to access an instance of that utility for a specific snippet, identified by element and/or id.

@pavelrappo
Copy link
Member Author

@pavelrappo pavelrappo commented Jul 26, 2021

  • All new top-level classes should have a reasonable documention comment, including where appropriate the standard boilerplate (i.e. not on public API classes and interfaces)
    • This is NOT part of any supported API.

    • If you write code that depends on this, you do so at your own risk.
    • This code and its internal interfaces are subject to change or
    • deletion without notice.
  • All new non-private methods should have a reasonable documentation comment

I'm ok with adding the boilerplate "This is NOT part of any supported API" note, as well as adding method comments where practical. However, I note that this PR comprises mostly an internal implementation and not a public API. Internal implementation comments are more like documentation rather than specification. Such documentation is virtually never built and is read in an IDE while eyeballing the respective code and tests. I think it's overkill and waste of resources to use doc comments in such a case.

  • There's more than a typical number of chatty comments and/or fixme comments and/or code that needs to be uncommented. I'll do a separate review pass to note some of those examples.

I'm currently working on removing as many FIXMEs and TODOs as possible. I think of a FIXME comment as a "review breakpoint" that highlights an issue that has to be resolved before the code is accepted.

I would be interested to learn which comments you consider "chatty". I look forward to fixing them with your help. One note, though. For an internal implementation, I try hard to make only the "why" comments, not the "what" comments. "What" comments have their place, but they should be sparing and reserved for non-trivial domains. For trivial domains, the "what" should be obvious. If it is not, I failed at coding, not at commenting.

@pavelrappo
Copy link
Member Author

@pavelrappo pavelrappo commented Jul 26, 2021

More feedback;

  1. It seems reasonable to introduce the ...taglets.snippets package. It seems excessive to have three sub packages within that, for actions, parser and text, especially when the last contains just a single class.

That package structure was initially there to keep the dependencies clear. That said, I'm okay with squashing it into a single package for practical reasons.

  1. At some point soonish, we will need API somewhere to get the content of the snippet. This is necessary to fulfill the goal in the JEP to support validation of snippet content. Without any such API, validation code would have to duplicate at least some of the code to handle snippets. This API should probably not be in com.sun.source and should probably be in the jdk.javadoc module, perhaps on the existing StandardDoclet class. One way to do that would be to introduce a utility interface to provide access to support for taglets, and then have a method on StandardDoclet to access an instance of that utility for a specific snippet, identified by element and/or id.

We can discuss that API either in this PR or in a separate thread on javadoc-dev at openjdk.java.net. (Separately: is this @/at trick still efficient? Modern email-address harvesting is surely smarter than that.)

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 26, 2021

Mailing list message from Jonathan Gibbons on javadoc-dev:

The comments don't need to be of core-libs quality, but at least some of
the problems we have had in the code of late has been discerning the
original intent, so at least some amount of comment/documentation is
useful, even if we don't run it through javadoc or even doclint.

Think of the comments as "pay it forward" to our future selves.

-- Jon

On 7/26/21 5:09 AM, Pavel Rappo wrote:

I'm ok with adding the boilerplate "This is NOT part of any supported API" note, as well as adding method comments where practical. However, I note that this PR comprises mostly an internal implementation and not a public API. Internal implementation comments are more like documentation rather than specification. Such documentation is virtually never built and is read in an IDE while eyeballing the respective code and tests. I think it's overkill and waste of resources to use doc comments in such a case.

Copy link
Member

@hns hns left a comment

Overall the code looks good!

A few comments inline. I'll have a look at the stylesheet changes next.

pavelrappo added 2 commits Jul 27, 2021
This commit adds trivial commentary to some of the Java declarations. This commit also adds the "This is NOT part of any supported API..." disclaimer.
@pavelrappo
Copy link
Member Author

@pavelrappo pavelrappo commented Jul 27, 2021

Mailing list message from Jonathan Gibbons on javadoc-dev:

The comments don't need to be of core-libs quality, but at least some of
the problems we have had in the code of late has been discerning the
original intent, so at least some amount of comment/documentation is
useful, even if we don't run it through javadoc or even doclint.

Think of the comments as "pay it forward" to our future selves.

-- Jon

On 7/26/21 5:09 AM, Pavel Rappo wrote:

I'm ok with adding the boilerplate "This is NOT part of any supported API" note, as well as adding method comments where practical. However, I note that this PR comprises mostly an internal implementation and not a public API. Internal implementation comments are more like documentation rather than specification. Such documentation is virtually never built and is read in an IDE while eyeballing the respective code and tests. I think it's overkill and waste of resources to use doc comments in such a case.

Project conventions are more important than my preferences. Although I don't see much value in such comments, I added some in commit 4300903.

pavelrappo added 2 commits Jul 27, 2021
This commit moves the contents of the jdk.javadoc.internal.doclets.toolkit.taglets.snippet.{action,parser,text} packages into the jdk.javadoc.internal.doclets.toolkit.taglets.snippet package.
pavelrappo added 6 commits Sep 3, 2021
DocCommentParser now recognizes end-of-input better. To test that in testBadTagSyntax, the ToolBox.writeJavaFiles approach was used instead that of ClassBuilder.write. This is because ClassBuilder muddled doc comments with extra formatting such as trailing newline. So from within `ClassBuilder.MethodBuilder.setComments(java.lang.String...)` one couldn't make an immediate end-of-input: there was always `\n` appended to the comment.

Also reformatted testBadTagSyntax for ease of comparing input and expected error output.
Improves code style; reformats.
Removes stale FIXMEs, TODOs and comments; downgrades FIXMEs to TODOs where possible; wraps overly long lines.
@openjdk openjdk bot removed the rfr label Sep 9, 2021
@openjdk openjdk bot added the rfr label Sep 9, 2021
Copy link
Member

@hns hns left a comment

I went through the changes again, they mostly look good. There are a few areas (mostly handling of tags and styled text) where I still want to do more exploring in order to better understand the code.

I left inline comments for minor issues/questions I encountered.

Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

The primary actionable item is that all strings that are intended for use in end-user error messages need to be localizable. i.e. see Parser.ParseException

Other comments are minor and can be deferred.

pavelrappo added 5 commits Sep 14, 2021
1. Reverts changes from CommentUtils.java (unused methods)
2. Changes misguided regex-related assertion to tests
 * Cleans up error string format
 * Internationalizes error messages, mapping them to fewer resource keys
 * Fixes a few bugs related to error messages, including: inaccurate caret positioning and runaway resource name (i.e. the resource key wasn't translated into an error)
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons left a comment

My primary concern in the last round of review was internationalization. You've addressed that: good.

When reading, and re-reading, a contribution this big, it's always possible to see new tiny details that could be improved. I've mentioned some here, and I'm sure that in times to come we will look at the code more and maybe see more tweaks. So, enough for now. Well done.

I note the varied references to improve JavadocTester.

# 0: path; 1: exception
doclet.error_setting_snippet_path=\
Error setting snippet path {0}: {1}
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add newline at end of file

Copy link
Member Author

@pavelrappo pavelrappo Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although it wasn't there to begin with, adding it won't be as big a deal as reformatting code in other places you suggested earlier.

protected boolean isUnquotedAttrValueTerminator(char ch) {
switch (ch) {
case ':': // indicates that the instruction relates to the next line
case ' ': case '\t':
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about other whitespace? Is that already covered?

Copy link
Member Author

@pavelrappo pavelrappo Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which other whitespace? If you are talking about \r or \n, then the string parsed by MarkupParser cannot have those. MarkupParser gets a single line without a trailing line terminator.

*
* This exception is only used to capture a user-facing error message.
* The message supplier is accepted not to control when to obtain a message,
* but to abstract how to obtain in.
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo "in"? maybe "one" or "it"?

Copy link
Member Author

@pavelrappo pavelrappo Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! It should've been "it".


/*
* While the "id" and "lang" attributes are advertised in JEP 413, they are
* currently unused by the implementation. The goal of this test is to make
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id's should be propagated to the output HTML

Copy link
Member Author

@pavelrappo pavelrappo Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About empty id's and propagating id's to the output HTML. I think we can address these in a follow-up issue. Would you be okay with it?

// This is because the below errors are modelled as exceptions thrown
// at parse time, when there are no doc trees yet. And the above errors
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would read better as "the errors below" and "the errors above"
https://www.grammarphobia.com/blog/2012/12/below.html

Copy link
Member Author

@pavelrappo pavelrappo Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

}

// TODO: perhaps this method could be added to JavadocTester
private void checkOutputEither(Output out, String first, String... other) {
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons Sep 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. This method allows a call with just a single String arg.
  2. Given that the method allows many String args, the name might be better as checkOneOf

Note to self/@jonathan-gibbons: review all references for suggestions to JavadocTester

@openjdk
Copy link

@openjdk openjdk bot commented Sep 21, 2021

@pavelrappo This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8266666: Implementation for snippets

Co-authored-by: Jonathan Gibbons <jjg@openjdk.org>
Co-authored-by: Hannes Wallnöfer <hannesw@openjdk.org>
Reviewed-by: jjg

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 4 new commits pushed to the master branch:

  • 42d5d2a: 8274056: JavaAccessibilityUtilities leaks JNI objects
  • 57df0db: 8270873: JFR: Catch DirectoryIteratorException when scanning for .jfr files
  • 111d5e1: 8273915: Create 'nosafepoint' rank
  • 7acec3f: 8236505: Mark jdk/editpad/EditPadTest.java as @headful

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Sep 21, 2021
@pavelrappo
Copy link
Member Author

@pavelrappo pavelrappo commented Sep 21, 2021

Thanks for approving the change, Jon. Some of your suggestions are added to 884dcb0.

@pavelrappo
Copy link
Member Author

@pavelrappo pavelrappo commented Sep 21, 2021

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Sep 21, 2021

Going to push as commit 0fc47e9.
Since your change was applied there have been 5 commits pushed to the master branch:

  • 6d91a3e: 8269039: Disable SHA-1 Signed JARs
  • 42d5d2a: 8274056: JavaAccessibilityUtilities leaks JNI objects
  • 57df0db: 8270873: JFR: Catch DirectoryIteratorException when scanning for .jfr files
  • 111d5e1: 8273915: Create 'nosafepoint' rank
  • 7acec3f: 8236505: Mark jdk/editpad/EditPadTest.java as @headful

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Sep 21, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Sep 21, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Sep 21, 2021

@pavelrappo Pushed as commit 0fc47e9.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler integrated javadoc
4 participants