RFR: 8332313: Update code review guidelines

[Kevin Rushforth]

On Wed, 15 May 2024 20:07:06 GMT, Phil Race <prr 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
>
> CONTRIBUTING.md line 233:
> 
>> 231: * Don't worry too much about import order. Try not to change it but don't worry about fighting your IDE to stop it from doing so.
>> 232: 
>> 233: New code should be formatted consistently in accordance with the above guidelines. However, please do not reformat existing code as part of a bug fix. The makes more changes for code reviewers to track and review, and can lead to merge conflicts. If you want to reformat a class, do that in a separate pull request (which will need its own unique JBS bug ID).
> 
> "The makes more changes" ? I think you mean "This" not "The"
> 
> I'm not sure about the last sentence, it seems to encourage reformatting fixes which are just noise most of the time.

Yeah, that was a typo (which I didn't notice when copying the block from the other doc). I'll fix it. And I agree with your concern, so I'll remove the last sentence.

> README-code-reviews.md line 14:
> 
>> 12: ### Reviewers
>> 13: 
>> 14: The [List of Reviewers](https://openjdk.java.net/census#openjfx) is on the OpenJDK Census.
> 
> We use ".org" now, not ".java.net"

Yes, I missed this. I'll update.

> README-code-reviews.md line 40:
> 
>> 38: ### 1. The Reviewer role for the OpenJFX Project
>> 39: 
>> 40: We define a formal "Reviewer" role, similar to the JDK project. A [Reviewer](https://openjdk.java.net/census#openjfx) is responsible for reviewing code changes and helping to determine whether a change is suitable for including into OpenJFX. We expect Reviewers to feel responsible not just for their piece, but for the quality of the JavaFX library as a whole. In other words, the role of Reviewer is one of stewardship. See the following section for what constitutes a good review.
> 
> (https://openjdk.java.net/census#openjfx)
> 
> .org please
> 
> BTW these very long source lines make it awkward to precisely identify the text I'm commenting on.

I'll fix it.

> README-code-reviews.md line 77:
> 
>> 75: 
>> 76: * All substantive feedback has been addressed, especially any objections from one with a Reviewer role.
>> 77: * All Reviewers who have requested the chance to review have done so (or indicated that they are OK with it going in without their review). In rare cases a Project Lead may override this.
> 
> One thing to add here (or hereabouts) is that if someone has commented on your review and requested changes that in almost all cases you should expect that they will want to return to review the results. So DO NOT push without letting earlier reviewers who made substantive comments re-review.

I'll add something about this.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602296959
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602297934
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602298847
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602310947