RFR: 8332313: Update code review guidelines

Phil Race prr at openjdk.org
Wed May 15 20:23:09 UTC 2024


On Wed, 15 May 2024 19:00:57 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Update the code review guidelines for JavaFX.
>> 
>> The JavaFX [CONTRIBUTING](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/CONTRIBUTING.md) guidelines includes guidance for creating, reviewing, and integrating changes to JavaFX, along with a pointer to a [Code Review Policies](https://wiki.openjdk.org/display/OpenJFX/Code+Reviews) 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:
>> 1. Converts the Code Review Policies Wiki page to a [README-code-reviews.md](https://github.com/kevinrushforth/jfx/blob/8332313-contributing/README-code-reviews.md) file in the repo and updates hyperlinks to the new location.
>> 2. Update `README-code-reviews.md` with new guidelines
>> 3. Update `CONTRIBUTING.md` to highlight important requirements  (and minor changes to `README-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.md` page to highlight important requirements
>
> README-code-reviews.md line 48:
> 
>> 46: All code reviews must be done via a pull request submitted against this GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID must exist before the pull request will be reviewed. See [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull request.
>> 47: 
>> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" role (aka a "R"eviewer). We have a different code review threshold for different types of changes. If there is disagreement as to whether a fix is low-impact or high-impact, then it is considered high-impact. In other words we will always err on the side of quality by "rounding up" to the next higher category. The contributor can say whether they think something is low-impact or high-impact, but It is up to a Reviewer to confirm this. A Reviewer either adds a comment indicating that they think a single review is sufficient, or else issues the Skara `/reviewers 2` command requesting a second reviewer (a Reviewer can request more than 2 reviewers in some cases where a fix might be especially risky or cut across multiple functional areas).
> 
> "but **It** is" -> it

I think it worth noting that in skara syntax that isn't two people with the reviewer role.
And tell people what to use if that is what they intend - eg if I have it right

-------------

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602190743


More information about the openjfx-dev mailing list