On Fri, 17 May 2024 14:10:43 GMT, Kevin Rushforth <k...@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 > > Kevin Rushforth has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 20 additional > commits since the last revision: > > - Wait for re-review if you've pushed a change addressing a substantive > comment > - Typo + remove sentence that invites reformatting PRs > - Wording changes, added example of API addition > - Formatting > - Add item about checking the target branch > - Remove trailing period from previous > - Minor changes regarding syncing from upstream master > - Clarify that GHA tests might fail for a reason unrelated to the PR > - Fix typo > > "but It" --> "but it" > - Remove bullet about checking the commit message (Skara already checks) > - ... and 10 more: https://git.openjdk.org/jfx/compare/1b088e5b...9cf7f920 README-code-reviews.md line 69: > 67: * Carefully consider the risk of regression > 68: * Carefully consider any compatibility concerns > 69: * Check whether it adds any new public or protected API, even implicitly > (such as a public method that overrides a protected method, or a class that > is moved from a non-exported to an exported package); if it does, indicate > that it needs a CSR I think that if it looks like an oversight (the author forgot about the default constructor), it's better to indicate that than to indicate that the PR needs a CSR. Maybe something like: "if it does and it doesn't look like an oversight, indicate that it needs a CSR" README-code-reviews.md line 72: > 70: * Focus first on substantive comments rather than stylistic comments > 71: * Check whether there is an automated test; if not, ask for one, if it is > feasible > 72: * Make sure that the PR has executed the GitHub Actions (GHA) tests; if > they aren't being run, ask the PR author to enable GHA workflows; if the test > fails on some platforms, check whether it is a real bug (sometimes a job > fails becau se of GHA infrastucture changes or we see a spurious GHA failure) becau se -> because ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605152176 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605147892
