RFR: 8332313: Update code review guidelines

[Andy Goryachev]

On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth <kcr 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 52:

> 50: 
> 51: * File a bug in JBS for every pull request
> 52: 

for line 53, would it make more sense to use this link
https://bugs.openjdk.org/projects/JDK/issues/
?

README-code-reviews.md line 10:

> 8: 
> 9: __Project Co-Lead__: Kevin Rushforth (kcr) <br>
> 10: __Project Co-Lead__: Johan Vos (jvos)

There are two sets of ids - one for OpenJFX/JBS and one for Github.  This might be confusing sometimes, should we list both?

README-code-reviews.md line 63:

> 61: Here is a list of things to keep in mind when reviewing a PR. This applies to anyone doing a review, but especially a "R"eviewer:
> 62: 
> 63: * Make sure you understand why there was an issue to begin with, and why/how the proposed PR solves the issue

Perhaps this should be noted as a required step for the PR author (maybe as a template text in the PR description?).

README-code-reviews.md line 64:

> 62: 
> 63: * Make sure you understand why there was an issue to begin with, and why/how the proposed PR solves the issue
> 64: * Focus first on substantive comments rather than stylistic comments

this is not as important as the next two.

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

regression and compatibility risks are probably the most important aspects of the code review from the "R"eviewer's perspective.  Perhaps this should be emphasized somehow?

README-code-reviews.md line 78:

> 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.
> 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business day (24 hours), not including weekends / or major holidays. This is to allow sufficient time for those reviewers who might be in other time zones the chance to review if they have concerns. This is measured from the time that Skara has most recently added the "rfr" label (for example, for a PR that was previously in Draft mode, wait for at least 24 hours after the PR has been taken out of Draft and marked "rfr"). In rare cases (e.g., a build breakage) a Reviewer might give the OK to integrate without waiting for 24 hours.

nit pick: 24 hours does not always equal to one business day.  A long weekend may extend the waiting period to 96 hours, a "winter break" might span the whole week.

README-code-reviews.md line 79:

> 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.
> 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business day (24 hours), not including weekends / or major holidays. This is to allow sufficient time for those reviewers who might be in other time zones the chance to review if they have concerns. This is measured from the time that Skara has most recently added the "rfr" label (for example, for a PR that was previously in Draft mode, wait for at least 24 hours after the PR has been taken out of Draft and marked "rfr"). In rare cases (e.g., a build breakage) a Reviewer might give the OK to integrate without waiting for 24 hours.
> 79: * Double-check the commit message before you integrate. Skara will list it near the top of your PR.

is this needed?  SKARA makes sure that it matches the JBS title.

README-code-reviews.md line 83:

> 81: #### A. Low-impact bug fixes.
> 82: 
> 83: These are typically isolated bug fixes with little or no impact beyond fixing the bug in question; included in this category are test fixes (including most new tests), doc fixes, and fixes to sample applications (including most new samples).

I think the main criteria is regression and compatibility impact.  Should we expand upon that here?

README-code-reviews.md line 99:

> 97: Feature requests come with a responsibility beyond just saying "here is the code for this cool new feature, please take it". There are many factors to consider for even small features. Larger features will need a significant contribution in terms of API design, coding, testing, maintainability, etc.
> 98: 
> 99: A feature should be discussed up-front on the openjfx-dev mailing list to get early feedback on the concept (is it a feature we are likely to accept) and the direction the API and/or implementation should take.

are there any special rules to gauging the consensus?  what happens when one party in a discussion stops responding?

README-code-reviews.md line 101:

> 99: A feature should be discussed up-front on the openjfx-dev mailing list to get early feedback on the concept (is it a feature we are likely to accept) and the direction the API and/or implementation should take.
> 100: 
> 101: To ensure that new features are consistent with the rest of the API and the desired direction of the Project, a CSR is required for a new Feature, API addition, or behavioral change. The CSR must be reviewed and approved by a "lead" of the Project. Currently this is either Kevin Rushforth or Johan Vos as indicated above.

this paragraph feels extremely nebulous.

a minor issue is that CSR cannot be written in full detail before the draft PR is created (a PoS is written),

a larger issue is that a decision to implement a feature might need a better process - is one lead approval sufficient?  what happens when leads disagree?

README-code-reviews.md line 103:

> 101: To ensure that new features are consistent with the rest of the API and the desired direction of the Project, a CSR is required for a new Feature, API addition, or behavioral change. The CSR must be reviewed and approved by a "lead" of the Project. Currently this is either Kevin Rushforth or Johan Vos as indicated above.
> 102: 
> 103: The review of the implementation follows the same "two reviewer" standard for higher-impact changes as described in category B. The two code reviewers for the implementation may or may not include the Lead who reviewed the CSR. The review / approval of the CSR is an independent step from the review / approval of the code change, although they can proceed in parallel.

it is not clear (at least to me) the required order in respect to CSR approval.  for a new feature, does the CSR need to be approved before the work can be started, or the draft PR is published, or an PR enters its "rfr" stage, or just before the integration?

Or perhaps it should be clarified that the CSR is just a human-readable, specially formatted specification of the *change*, and it is an integral part of the new feature that needs to be approved before the feature can be integrated?

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602050560
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602061399
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602072022
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602072829
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602075277
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602082570
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602083761
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602085558
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602098222
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602095257
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602105550