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

JDK-8267204: Expose access to underlying streams in Reporter #4216

Closed

Conversation

jonathan-gibbons
Copy link
Contributor

@jonathan-gibbons jonathan-gibbons commented May 27, 2021

Please review an update to jdk.javadoc/jdk.javadoc.doclets.Reporter to add 3 new methods and to improve the descriptions of other parts of the interface.

The new methods provide access to the underlying streams (informally, for standard output and diagnostic output), and a new report method to report diagnostics at an arbitrary position in a file being read by a doclet, or a taglet within a doclet.


Progress

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

Issue

  • JDK-8267204: Expose access to underlying streams in Reporter

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4216

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

Using diff file

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

@bridgekeeper
Copy link

bridgekeeper bot commented May 27, 2021

👋 Welcome back jjg! 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
Copy link

openjdk bot commented May 27, 2021

@jonathan-gibbons The following label will be automatically applied to this pull request:

  • javadoc

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

@openjdk openjdk bot added the javadoc javadoc-dev@openjdk.org label May 27, 2021
@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented May 27, 2021

/csr

@openjdk openjdk bot added the csr Pull request needs approved CSR before integration label May 27, 2021
@openjdk
Copy link

openjdk bot commented May 27, 2021

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

Copy link
Member

@pavelrappo pavelrappo left a comment

Thank you so much for doing this, Jon. Although this PR is good by itself, it will be particularly useful for snippets (JEP 413). While I'm still reviewing this PR, you could have a look at the comments I have made so far.

* This interface provides error, warning and notice reporting.
* Interface for reporting diagnostics and other messages.
*
* <p>Diagnostics consist of a diagnostic {@link Diagnostic.Kind kind} and a message,
Copy link
Member

@pavelrappo pavelrappo May 27, 2021

Choose a reason for hiding this comment

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

Consider moving "diagnostic" into the @link label: {@link Diagnostic.Kind diagnostic kind}.

* Prints a diagnostic message.
*
* @param kind the kind of diagnostic
* @param message message to print
Copy link
Member

@pavelrappo pavelrappo May 27, 2021

Choose a reason for hiding this comment

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

Prepend "message to print" with "the". Alternatively, consider using "the message to be printed", which seems typical in this file.

* @param pos the position to be associated with the end
* @param end the end of a range of characters to be associated with the end
* @param key the name of a resource containing the message to be printed
* @param args optional arguments to be replaced in the message.
Copy link
Member

@pavelrappo pavelrappo May 27, 2021

Choose a reason for hiding this comment

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

Remove the trailing period.

* @param pos the position to be associated with the end
* @param end the end of a range of characters to be associated with the end
* @param key the name of a resource containing the message to be printed
* @param args optional arguments to be replaced in the message.
Copy link
Member

@pavelrappo pavelrappo May 27, 2021

Choose a reason for hiding this comment

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

Same here: remove the trailing period.

@@ -389,16 +432,21 @@ public boolean hasWarnings() {
}

/**
* Prints the error and warning counts, if any.
* Prints the error and warning counts, if any, to. the diagnostic writer
Copy link
Member

@pavelrappo pavelrappo May 27, 2021

Choose a reason for hiding this comment

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

Move period from after "to" to the end of the sentence.

Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons May 27, 2021

Choose a reason for hiding this comment

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

Wow, how did that typo come about? :-)

* @param start the start of a range of characters to be associated with the end
* @param pos the position to be associated with the end
* @param end the end of a range of characters to be associated with the end
Copy link
Member

@pavelrappo pavelrappo May 27, 2021

Choose a reason for hiding this comment

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

"Associated with end"? I believe it should either be "error" or "message", not "end".

* @param start the start of a range of characters to be associated with the end
* @param pos the position to be associated with the end
* @param end the end of a range of characters to be associated with the end
Copy link
Member

@pavelrappo pavelrappo May 27, 2021

Choose a reason for hiding this comment

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

Same here: "associated with the end"?

@pavelrappo
Copy link
Member

pavelrappo commented May 27, 2021

As far as I understand #4077 will be closed as superseded by this PR?

@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented May 27, 2021

As far as I understand #4077 will be closed as superseded by this PR?

Yes, I've closed the PR.

@jonathan-gibbons jonathan-gibbons marked this pull request as ready for review Jun 2, 2021
@openjdk openjdk bot added rfr Pull request is ready for review and removed csr Pull request needs approved CSR before integration labels Jun 2, 2021
@mlbridge
Copy link

mlbridge bot commented Jun 2, 2021

Webrevs

Copy link
Member

@pavelrappo pavelrappo left a comment

Update the copyright years and you're good to go.

* <p>Diagnostics consist of a {@link Diagnostic.Kind diagnostic kind} and a message,
* and may additionally be associated with an {@link Element element},
* a {@link DocTreePath tree node} in a documentation comment,
* or at an arbitrary position in a given {@link FileObject file}.
Copy link
Member

@pavelrappo pavelrappo Jun 4, 2021

Choose a reason for hiding this comment

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

"at" looks misplaced here.

Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons Jun 4, 2021

Choose a reason for hiding this comment

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

When I wrote it, it looked right; now, I agree with you.



Copy link
Member

@pavelrappo pavelrappo Jun 4, 2021

Choose a reason for hiding this comment

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

Delete one line.

* @param e an element identifying the declaration whose position should
* be included with the message
Copy link
Member

@pavelrappo pavelrappo Jun 4, 2021

Choose a reason for hiding this comment

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

I see what you've changed: one of these words needed to go. Either "should" or "to". You chose "to". Unless you did it for semantical reasons, I note that this file uses "to be" in the vast majority of similar cases.

Copy link
Contributor Author

@jonathan-gibbons jonathan-gibbons Jun 4, 2021

Choose a reason for hiding this comment

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

The wording is correct, but I agree it the overall phrasing is inconsistent in form with similar phrases elsewhere, because in this context it uses "the declaration whose position ..."

It's an internal API, so I don't want to get too hung up on it. I'll change the form to be more like the others.

@openjdk
Copy link

openjdk bot commented Jun 4, 2021

@jonathan-gibbons 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:

8267204: Expose access to underlying streams in Reporter

Reviewed-by: prappo

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 1 new commit pushed to the master branch:

  • 48dc72b: 8268272: Remove JDK-8264874 changes because Graal was removed.

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 Pull request is ready to be integrated label Jun 4, 2021
@jonathan-gibbons
Copy link
Contributor Author

jonathan-gibbons commented Jun 5, 2021

/integrate

@openjdk openjdk bot closed this Jun 5, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Jun 5, 2021
@openjdk
Copy link

openjdk bot commented Jun 5, 2021

@jonathan-gibbons Since your change was applied there have been 3 commits pushed to the master branch:

  • 76b54a1: 8263512: [macos_aarch64] issues with calling va_args functions from invoke_native
  • 4e6748c: 8267687: ModXNode::Ideal optimization is better than Parse::do_irem
  • 48dc72b: 8268272: Remove JDK-8264874 changes because Graal was removed.

Your commit was automatically rebased without conflicts.

Pushed as commit 6ff978a.

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

@jonathan-gibbons jonathan-gibbons deleted the jdk-8267204-reporter branch Oct 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
integrated Pull request has been integrated javadoc javadoc-dev@openjdk.org
3 participants