jfx
jfx copied to clipboard
8332313: Update code review guidelines
Update the code review guidelines for JavaFX.
The JavaFX CONTRIBUTING guidelines includes guidance for creating, reviewing, and integrating changes to JavaFX, along with a pointer to a Code Review Policies Wiki page.
This PR updates these guidelines to improve the quality of reviews, with a goal of improving JavaFX and decreasing the chance of introducing a serious regression or other critical bug.
The source branch has three commits:
- Converts the Code Review Policies Wiki page to a README-code-reviews.md file in the repo and updates hyperlinks to the new location.
- Update
README-code-reviews.mdwith new guidelines - Update
CONTRIBUTING.mdto highlight important requirements (and minor changes toREADME-code-reviews.md)
Commit 1 is content neutral, so it might be helpful for reviewers to look at the changes starting from the second commit.
The updates are:
- In the Overview section, add a list of items for Reviewers, PR authors, and sponsoring Committers to verify prior to integration
- Create a "Guidelines for reviewing a PR" subsection of the Code review policies section
- Create a "Before you integrate or sponsor a PR" subsection of the Code review policies section
- Update the
CONTRIBUTING.mdpage to highlight important requirements
Progress
- [x] Change must not contain extraneous whitespace
- [x] Commit message must refer to an issue
- [ ] Change must be properly reviewed (3 reviews required, with at least 3 Reviewers)
Issue
- JDK-8332313: Update code review guidelines (Enhancement - P3)
Reviewers
- John Hendrikx (@hjohn - Committer) 🔄 Re-review required (review applies to 1de03ec7)
- Andy Goryachev (@andy-goryachev-oracle - Reviewer)
- Nir Lisker (@nlisker - Reviewer)
Reviewing
Using git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1455/head:pull/1455
$ git checkout pull/1455
Update a local copy of the PR:
$ git checkout pull/1455
$ git pull https://git.openjdk.org/jfx.git pull/1455/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1455
View PR using the GUI difftool:
$ git pr show -t 1455
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1455.diff
Webrev
:wave: Welcome back kcr! 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.
@kevinrushforth 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:
8332313: Update code review guidelines
Reviewed-by: jhendrikx, angorya, nlisker, jvos
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 3 new commits pushed to the master branch:
- b685db23f07df32a3caea7af36206c48b52bb6eb: 8313138: Scrollbar Keyboard enhancement
- b5fe362286056e516be1d26f5d1cdda12eb20a4c: 8330590: TextInputControl: previous word fails with Bhojpuri characters
- 9dc4aa2341581d730c9d721e91ac0da081ffddcc: 8324327: ColorPicker shows a white rectangle on clicking on picker
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.
Reviewers: @johanvos @andy-goryachev-oracle @arapte @prrace
I'd like at least 3 "R"eviewers to review this. Committers and Authors are also welcome to provide feedback.
/reviewers 3 reviewers
@kevinrushforth The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 3 (with at least 3 Reviewers).
Webrevs
- 03: Full - Incremental (7725881b)
- 02: Full - Incremental (784475b0)
- 01: Full - Incremental (9cf7f920)
- 00: Full (1de03ec7)
I addressed most of the feedback. There are still one or two more items I might update for this PR, but I'll capture the rest in follow-up issues.
I filed the following follow-on bug to address and pending items:
JDK-8332642: Additional improvements to contribution and code review guidelines
I think I have addressed all of the feedback so far, either by updating the docs or by adding an item to JDK-8332642.
@johanvos Do you want to review this?
/integrate
Going to push as commit 94aa2b68d01af6096a18316ab36c911fe8cec572.
Since your change was applied there have been 3 commits pushed to the master branch:
- b685db23f07df32a3caea7af36206c48b52bb6eb: 8313138: Scrollbar Keyboard enhancement
- b5fe362286056e516be1d26f5d1cdda12eb20a4c: 8330590: TextInputControl: previous word fails with Bhojpuri characters
- 9dc4aa2341581d730c9d721e91ac0da081ffddcc: 8324327: ColorPicker shows a white rectangle on clicking on picker
Your commit was automatically rebased without conflicts.
@kevinrushforth Pushed as commit 94aa2b68d01af6096a18316ab36c911fe8cec572.
:bulb: You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.