RFR: 8332313: Update code review guidelines

[Andy Goryachev]

On Wed, 15 May 2024 19:23:25 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 66:
> 
>> 64: * Focus first on substantive comments rather than stylistic comments
>> 65: * Consider the risk of regression
>> 66: * Consider any compatibility concerns
> 
> This might be included in this point already, but one thing I sometimes miss is the inadvertent introduction of new API (that will have to be deprecated if missed). This can happen with `protected` methods, default `public` constructors (esp. if a non-API constructor is removed), or if a class is moved from a non-exported to an exported package (something that you can't see by looking at the area you're reviewing, you need to check the `module-info`, or more practically, look at the package names).

I was wondering if there ought to be an unofficial checklist for things to look for?
As new people become "R"eviewers, I think there is a value in accumulating collective wisdom in a checklist.  I like checklists.

> README-code-reviews.md line 68:
> 
>> 66: * Consider any compatibility concerns
>> 67: * Check whether there is an automated test; if not, ask for one, if it is feasible
>> 68: * Make sure that the PR has executed the GHA tests and that they all pass; if they aren't being run, ask the PR author to enable GHA workflows
> 
> These tests sometimes fail and we integrate anyway. I noticed that sometimes they need a few iterations to succeed. Are we really dependent on them?
> 
> Edit: currently the Linux test is failing, and this PR can't be the reason.

or is it?  :-)

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

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