On Wed, 15 May 2024 19:04:19 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 48: > >> 46: All code reviews must be done via a pull request submitted against this >> GitHub repo, [openjdk/jfx](https://github.com/openjdk/jfx). A JBS bug ID >> must exist before the pull request will be reviewed. See >> [CONTRIBUTING.md](CONTRIBUTING.md) for information on how to submit a pull >> request. >> 47: >> 48: All fixes must be reviewed by at least one reviewer with the "Reviewer" >> role (aka a "R"eviewer). We have a different code review threshold for >> different types of changes. If there is disagreement as to whether a fix is >> low-impact or high-impact, then it is considered high-impact. In other words >> we will always err on the side of quality by "rounding up" to the next >> higher category. The contributor can say whether they think something is >> low-impact or high-impact, but It is up to a Reviewer to confirm this. A >> Reviewer either adds a comment indicating that they think a single review is >> sufficient, or else issues the Skara `/reviewers 2` command requesting a >> second reviewer (a Reviewer can request more than 2 reviewers in some cases >> where a fix might be especially risky or cut across multiple functional >> areas). > > About requesting reviews. I think that only some people can request reviews > through GitHub, I never managed to do it on this repo, probably a matter of > permissions. Might worth clarifying how this works. It's better to just add comments `@` mentioning who you want to do the review, since there isn't a way for most people to request a review from others. > README-code-reviews.md line 58: > >> 56: * Determine whether this needs 2 reviewers and whether it needs a CSR; >> issue the `/reviewers 2` or `/csr` command as needed >> 57: * If you want to indicate your approval, but still feel additional >> reviewers are needed, you may increase the number of reviewers (e.g., from 2 >> to 3) >> 58: * If you want an area expert to review a PR, indicate this in a comment >> of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can >> indicate whether or not they plan to review it > > Should a list of experts per area be made available somewhere? The Wiki has > an old "code ownership" table that is out of date. Usually you get to know > the experts only after they have reviewed your code a couple of times. That might be something to consider as a follow-on. > README-code-reviews.md line 60: > >> 58: * If you want an area expert to review a PR, indicate this in a comment >> of the form: `Reviewers: @PERSON1 @PERSON2`; the requested reviewers can >> indicate whether or not they plan to review it >> 59: * If you want to ensure that you have the opportunity to review this PR >> yourself, add a comment of the form: `@PRAUTHOR Wait for me to review this >> PR`, optionally add any concerns you might have >> 60: > > Another thing to look out for is the targeting branch during rampdown. Should > they all target master and then be backported as needed, or can some target > the rampdown branch? Good idea. I'll add the bit about checking the target branch -- if the PR isn't a backport it should almost always target the master branch (there might be _very_ rare exception where a bug is specific to the stabilization branch). > README-code-reviews.md line 69: > >> 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 >> 69: * If the PR source branch hasn't synced up from master in a long time, >> or if there is an upstream commit not in the source branch that might >> interfere with the PR, ask the PR author to merge the latest upstream master. > > This is the only bullet point with a period at the end. I'll fix it. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602301589 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602301960 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602303525 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602310146
