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