On Wed, 15 May 2024 19:23:25 GMT, Nir Lisker <nlis...@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
