RFR: 8332313: Update code review guidelines

Wed May 15 19:46:13 UTC 2024

On Wed, 15 May 2024 18:20:22 GMT, Andy Goryachev <angorya 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 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?).

I like the idea of adding some additional information on "here's what makes a good PR" to the CONTRIBUTING guidelines. I'll either take a stab at this or else file a follow-on issue.

> 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.

Agreed. I'll move this bullet down.

> 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?

That's a good idea.

> 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?

That seems like a good idea.

> 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?

This is out of scope for this PR (this section is unchanged). It might be something to consider addressing in a follow-on enhancement, although I'm not sure I want to make it any more specific.

> 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?

This is out of scope for this PR, but it might be something to consider addressing in a follow-on enhancement.

> 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?

This is out of scope for this PR, but it might be something to consider addressing in a follow-on enhancement.

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602156743
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602157447
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602157689
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602158932
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602148854
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602150110
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602150492