-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
8332071: Convert package.html files in java.management.rmi
to package-info.java
#19263
Conversation
👋 Welcome back nizarbenalla! A progress list of the required criteria for merging this PR into |
@nizarbenalla 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:
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 108 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@kevinjwalls, @RogerRiggs) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
@nizarbenalla The following labels will be automatically applied to this pull request:
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. |
/issue add JDK-8332286 |
@nizarbenalla |
java.management.rmi
to package-info.java
- Add `@since` tags to RMIConnectorServer
a26ee08
to
8a644f3
Compare
sorry about the force push, had to. |
/issue add JDK-8332376 |
/issue remove JDK-8332286 |
@nizarbenalla |
@nizarbenalla |
/label remove jmx |
@nizarbenalla |
Webrevs
|
* questions. | ||
*/ | ||
|
||
/** |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you'll need to prepend each line with *
too, which has the side effect of making it appear that every line is changed but I think we just need to get over that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing that makes git think it's a new file, rather than a rename.
I was doing this in a26ee08 and removed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM - there are further potential improvements that could be made in this file - like replacing <code></code>
with {@code }
and <pre></pre>
with {@snippet }
but I guess that can wait until someone has the inclination and bandwidth to do it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, found how to "lazily" append to the lines I want so adding the " *" wasn't an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good - with the asterisk * prefix question from the other comment.
I looked to check and yes the since 10 is correct.
/reviewers 2 |
@kevinjwalls |
* | ||
* @since 1.5 | ||
* | ||
**/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra * */
is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 393bf3a, thanks.
* | ||
* @since 10 | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix the indentation of the "*" for the this comment on CREDENTIALS_FILTER_PATTERN so it looks consistent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed to be consistent, but the rest of the comment is still indented wrong.
/**
* Name of the attribute that specifies ...
* ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I would have gone the other way and corrected the preceding indentation to the standard).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/integrate |
@nizarbenalla |
/sponsor |
Going to push as commit a0c5714.
Your commit was automatically rebased without conflicts. |
@kevinjwalls @nizarbenalla Pushed as commit a0c5714. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review this change. I converted the
package.html
file topackage-info.java
, becausejavac
cannot recognizepackage.html
.I already brought this up in the mailing list.
The conversion was done in-place, only renaming it in git.
I also added a couple of
@since
tags, with only 2 changes I don't want to split these two fixes into separate PRs.CREDENTIALS_FILTER_PATTERN
andSERIAL_FILTER_PATTERN
were first added in https://bugs.openjdk.org/browse/JDK-8187556Progress
Issues
java.management.rmi
to package-info.java (Sub-task - P4)@<!---->since
tags tojava.management.rmi
(Sub-task - P4)Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19263/head:pull/19263
$ git checkout pull/19263
Update a local copy of the PR:
$ git checkout pull/19263
$ git pull https://git.openjdk.org/jdk.git pull/19263/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19263
View PR using the GUI difftool:
$ git pr show -t 19263
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19263.diff
Webrev
Link to Webrev Comment