Re: RFR: 8332313: Update code review guidelines [v4]
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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 incrementally with one > additional commit since the last revision: > > Clarify need for CSR when API is modified or removed as well as added Overall, this is a great improvement. Reviewing code is a huge responsibility and it's good that we make it more clear now how we expect reviews to be executed. - Marked as reviewed by jvos (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2071576062
Re: RFR: 8332313: Update code review guidelines [v4]
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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 incrementally with one > additional commit since the last revision: > > Clarify need for CSR when API is modified or removed as well as added @johanvos Do you want to review this? - PR Comment: https://git.openjdk.org/jfx/pull/1455#issuecomment-2124636248
Re: RFR: 8332313: Update code review guidelines [v4]
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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 incrementally with one > additional commit since the last revision: > > Clarify need for CSR when API is modified or removed as well as added Marked as reviewed by jhendrikx (Committer). - PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2070822120
Re: RFR: 8332313: Update code review guidelines [v4]
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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 incrementally with one > additional commit since the last revision: > > Clarify need for CSR when API is modified or removed as well as added Marked as reviewed by nlisker (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2069892902
Re: RFR: 8332313: Update code review guidelines [v4]
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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 incrementally with one > additional commit since the last revision: > > Clarify need for CSR when API is modified or removed as well as added I filed the following follow-on bug to address and pending items: [JDK-8332642](https://bugs.openjdk.org/browse/JDK-8332642): Additional improvements to contribution and code review guidelines I think I have addressed all of the feedback so far, either by updating the docs or by adding an item to JDK-8332642. - PR Comment: https://git.openjdk.org/jfx/pull/1455#issuecomment-2123544298
Re: RFR: 8332313: Update code review guidelines [v2]
On Sat, 18 May 2024 14:28:31 GMT, Nir Lisker wrote: >> We have by now cleaned up our public API to avoid classes with an implicit >> no-arg constructor, so the only way this situation could arise in the future >> is if someone adds a new public class, which needs a CSR anyway. >> >> I guess it could also happen if someone proposed to delete the no-arg >> constructor, but we don't allow deleting of non-terminally-deprecated API >> anyway. Maybe it's worth changing "adds any new" to "adds, removes, or >> modifies any"? >> >> Somewhat related, these guidelines don't address what to look for when >> reviewing new API; perhaps a follow-on issue. > >> Maybe it's worth changing "adds any new" to "adds, removes, or modifies any"? > > This looks like a good idea. Done. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1609019119
Re: RFR: 8332313: Update code review guidelines [v4]
On Tue, 21 May 2024 22:32:21 GMT, Kevin Rushforth 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 incrementally with one > additional commit since the last revision: > > Clarify need for CSR when API is modified or removed as well as added Marked as reviewed by angorya (Reviewer). - PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2069743281
Re: RFR: 8332313: Update code review guidelines [v4]
> 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 incrementally with one additional commit since the last revision: Clarify need for CSR when API is modified or removed as well as added - Changes: - all: https://git.openjdk.org/jfx/pull/1455/files - new: https://git.openjdk.org/jfx/pull/1455/files/784475b0..7725881b Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1455=03 - incr: https://webrevs.openjdk.org/?repo=jfx=1455=02-03 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1455.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1455/head:pull/1455 PR: https://git.openjdk.org/jfx/pull/1455
Re: RFR: 8332313: Update code review guidelines [v2]
On Sat, 18 May 2024 14:24:49 GMT, Kevin Rushforth wrote: > Maybe it's worth changing "adds any new" to "adds, removes, or modifies any"? This looks like a good idea. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605798488
Re: RFR: 8332313: Update code review guidelines [v2]
On Fri, 17 May 2024 15:02:14 GMT, Nir Lisker wrote: >> 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/1a4c0fd4...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" We have by now cleaned up our public API to avoid classes with an implicit no-arg constructor, so the only way this situation could arise in the future is if someone adds a new public class, which needs a CSR anyway. I guess it could also happen if someone proposed to delete the no-arg constructor, but we don't allow deleting of non-terminally-deprecated API anyway. Maybe it's worth changing "adds any new" to "adds, removes, or modifies any"? Somewhat related, these guidelines don't address what to look for when reviewing new API; perhaps a follow-on issue. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605797980
Re: RFR: 8332313: Update code review guidelines [v2]
On Fri, 17 May 2024 14:58:41 GMT, Nir Lisker wrote: >> 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 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 Never mind, got simul-fixed. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605160705
Re: RFR: 8332313: Update code review guidelines [v2]
On Fri, 17 May 2024 14:10:43 GMT, Kevin Rushforth 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
Re: RFR: 8332313: Update code review guidelines [v3]
On Fri, 17 May 2024 14:55:18 GMT, Kevin Rushforth 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 incrementally with one > additional commit since the last revision: > > Fix typos was going to comment on two spelling errors, but you fixed it. looks good, thank you for moving this doc to the main repo as a markdown file! - Marked as reviewed by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2063681694
Re: RFR: 8332313: Update code review guidelines [v3]
> 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 incrementally with one additional commit since the last revision: Fix typos - Changes: - all: https://git.openjdk.org/jfx/pull/1455/files - new: https://git.openjdk.org/jfx/pull/1455/files/9cf7f920..784475b0 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1455=02 - incr: https://webrevs.openjdk.org/?repo=jfx=1455=01-02 Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod Patch: https://git.openjdk.org/jfx/pull/1455.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1455/head:pull/1455 PR: https://git.openjdk.org/jfx/pull/1455
Re: RFR: 8332313: Update code review guidelines [v2]
On Fri, 17 May 2024 14:10:43 GMT, Kevin Rushforth 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/b855c59f...9cf7f920 I addressed most of the feedback. There are still one or two more items I might update for this PR, but I'll capture the rest in follow-up issues. - PR Comment: https://git.openjdk.org/jfx/pull/1455#issuecomment-2117754680
Re: RFR: 8332313: Update code review guidelines [v2]
On Wed, 15 May 2024 22:18:24 GMT, Kevin Rushforth wrote: >> README-code-reviews.md line 79: >> >>> 77: * All Reviewers who have requested the chance to review have done so >>> (or indicated that they are OK with it going in without their review). In >>> rare cases a Project Lead may override this. >>> 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business >>> day (24 hours), not including weekends or major holidays. This is to allow >>> sufficient time for those reviewers who might be in other time zones the >>> chance to review if they have concerns. This is measured from the time that >>> Skara has most recently added the "rfr" label (for example, for a PR that >>> was previously in Draft mode, wait for at least 24 hours after the PR has >>> been taken out of Draft and marked "rfr"). In rare cases (e.g., a build >>> breakage) a Reviewer might give the OK to integrate without waiting for 24 >>> hours. >>> 79: * Verify the commit message. The Skara tooling adds a comment near the >>> top of the PR with the commit message that will be used. You can add a >>> summary to the commit message with the `/summary` command. You can add >>> additional contributors with the `/contributor` command. Commands are >>> issued by adding a comment to the PR that starts with a slash `/` character. >> >> The last sentence feels a bit out of place, as several places earlier in >> this document already mention slash commands. > > I'll remove it (I moved it from the CONTRIBUTING doc, and it does seem out > of place). > > Actually as someone else commented, Skara will verify that the PR title and > JBS bug match, so mostly this is about adding a summary and/or contributors > if you want to. Removed. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605063652
Re: RFR: 8332313: Update code review guidelines [v2]
On Thu, 16 May 2024 07:27:34 GMT, Johan Vos wrote: >> That's a good idea. > > In the ideal world where we have tons of regression and compatibility tests, > I would agree. Unfortunately, we are totally not there yet. Compared to other > projects, the quality of tests in OpenJFX is good, but given the multitude of > usage scenario's in client development, we would need much, much more tests > before we can rely on them to detect regression. > Therefore, the #1 point imho is that both the author (as Andy commented) and > the reviewer have a real good understanding on what is happening. Every > single change in a PR should be understood. The most dangerous things are "I > don't understand why, but it seems to fail before and succeed after the > patch". Yes, this a good point. I left the "make sure you understand..." bullet as the first, and moved the "Focus first on substantive comments" below the regression and compatibility bullets. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605059984
Re: RFR: 8332313: Update code review guidelines [v2]
On Wed, 15 May 2024 18:30:06 GMT, Andy Goryachev wrote: >> 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/3f56e6c0...9cf7f920 > > README-code-reviews.md line 78: > >> 76: * All substantive feedback has been addressed, especially any objections >> from one with a Reviewer role. >> 77: * All Reviewers who have requested the chance to review have done so (or >> indicated that they are OK with it going in without their review). In rare >> cases a Project Lead may override this. >> 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business >> day (24 hours), not including weekends / or major holidays. This is to allow >> sufficient time for those reviewers who might be in other time zones the >> chance to review if they have concerns. This is measured from the time that >> Skara has most recently added the "rfr" label (for example, for a PR that >> was previously in Draft mode, wait for at least 24 hours after the PR has >> been taken out of Draft and marked "rfr"). In rare cases (e.g., a build >> breakage) a Reviewer might give the OK to integrate without waiting for 24 >> hours. > > nit pick: 24 hours does not always equal to one business day. A long weekend > may extend the waiting period to 96 hours, a "winter break" might span the > whole week. fixed > README-code-reviews.md line 79: > >> 77: * All Reviewers who have requested the chance to review have done so (or >> indicated that they are OK with it going in without their review). In rare >> cases a Project Lead may override this. >> 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business >> day (24 hours), not including weekends / or major holidays. This is to allow >> sufficient time for those reviewers who might be in other time zones the >> chance to review if they have concerns. This is measured from the time that >> Skara has most recently added the "rfr" label (for example, for a PR that >> was previously in Draft mode, wait for at least 24 hours after the PR has >> been taken out of Draft and marked "rfr"). In rare cases (e.g., a build >> breakage) a Reviewer might give the OK to integrate without waiting for 24 >> hours. >> 79: * Double-check the commit message before you integrate. Skara will list >> it near the top of your PR. > > is this needed? SKARA makes sure that it matches the JBS title. removed bullet - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605060203 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605060466
Re: RFR: 8332313: Update code review guidelines [v2]
On Wed, 15 May 2024 19:38:55 GMT, Kevin Rushforth wrote: >> 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. fixed - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605057721
Re: RFR: 8332313: Update code review guidelines [v2]
On Wed, 15 May 2024 22:00:42 GMT, Kevin Rushforth wrote: >> I think it worth noting that in skara syntax that isn't two people with the >> reviewer role. >> And tell people what to use if that is what they intend - eg if I have it >> right > > It's `/reviewers 2 reviewers`, so I'll add that as an example and clarify > that `/reviewers 2` is 2 with at least one reviewer. Fixed (both the example Phil asked for and the typo Nir noted) - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605061762
Re: RFR: 8332313: Update code review guidelines [v2]
On Wed, 15 May 2024 21:58:46 GMT, Kevin Rushforth wrote: >> README-code-reviews.md line 14: >> >>> 12: ### Reviewers >>> 13: >>> 14: The [List of Reviewers](https://openjdk.java.net/census#openjfx) is on >>> the OpenJDK Census. >> >> We use ".org" now, not ".java.net" > > Yes, I missed this. I'll update. fixed >> README-code-reviews.md line 40: >> >>> 38: ### 1. The Reviewer role for the OpenJFX Project >>> 39: >>> 40: We define a formal "Reviewer" role, similar to the JDK project. A >>> [Reviewer](https://openjdk.java.net/census#openjfx) is responsible for >>> reviewing code changes and helping to determine whether a change is >>> suitable for including into OpenJFX. We expect Reviewers to feel >>> responsible not just for their piece, but for the quality of the JavaFX >>> library as a whole. In other words, the role of Reviewer is one of >>> stewardship. See the following section for what constitutes a good review. >> >> (https://openjdk.java.net/census#openjfx) >> >> .org please >> >> BTW these very long source lines make it awkward to precisely identify the >> text I'm commenting on. > > I'll fix it. fixed >> README-code-reviews.md line 77: >> >>> 75: >>> 76: * All substantive feedback has been addressed, especially any >>> objections from one with a Reviewer role. >>> 77: * All Reviewers who have requested the chance to review have done so >>> (or indicated that they are OK with it going in without their review). In >>> rare cases a Project Lead may override this. >> >> One thing to add here (or hereabouts) is that if someone has commented on >> your review and requested changes that in almost all cases you should expect >> that they will want to return to review the results. So DO NOT push without >> letting earlier reviewers who made substantive comments re-review. > > I'll add something about this. fixed - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605064310 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605064515 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605064857
Re: RFR: 8332313: Update code review guidelines [v2]
On Wed, 15 May 2024 22:13:34 GMT, Kevin Rushforth wrote: >>> or is it? :-) >> >>  >> >> Looks to me like it is... > > A passing GHA test run is neither necessary nor sufficient. It is an > interesting data point. A PR can be integrated with a failing run as long as > we understand why it failed. It is something for a reviewer to check and ask > about if they suspect it points to a real problem. I'll reword this a little > bit to make it clear. I'm sort of glad that GitHub / Azure chose today to > break it. It is a great illustration of why we don't want it to be blocking. > > Btw, I filed https://bugs.openjdk.org/browse/JDK-8332328 to track the GHA > failure. I updated the wording slightly - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605062114
Re: RFR: 8332313: Update code review guidelines [v2]
On Wed, 15 May 2024 22:10:12 GMT, Kevin Rushforth wrote: > > ...inadvertent introduction of new API (that will have to be deprecated if > > missed) > > I think this is worth mentioning. Fixed. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605063298
Re: RFR: 8332313: Update code review guidelines [v2]
On Wed, 15 May 2024 22:05:46 GMT, Kevin Rushforth wrote: >> 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). fixed >> 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. fixed - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605062508 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605062271
Re: RFR: 8332313: Update code review guidelines [v2]
On Wed, 15 May 2024 21:57:55 GMT, Kevin Rushforth wrote: >> exactly my point. when the majority of time is spent in the context of >> github (PR review etc) it might be confusing when one types `@jvos` and it's >> not found, or when within a PR there are references to both `jvos` and >> `johanvos` >> >> (see the very first comment in https://github.com/openjdk/jfx/pull/1388 ) > > Ah, I misunderstood Andy's comment. Yes, I'll update this. fixed - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605057420
Re: RFR: 8332313: Update code review guidelines [v2]
> 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/3f56e6c0...9cf7f920 - Changes: - all: https://git.openjdk.org/jfx/pull/1455/files - new: https://git.openjdk.org/jfx/pull/1455/files/1de03ec7..9cf7f920 Webrevs: - full: https://webrevs.openjdk.org/?repo=jfx=1455=01 - incr: https://webrevs.openjdk.org/?repo=jfx=1455=00-01 Stats: 51497 lines in 127 files changed: 26323 ins; 14206 del; 10968 mod Patch: https://git.openjdk.org/jfx/pull/1455.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1455/head:pull/1455 PR: https://git.openjdk.org/jfx/pull/1455
Re: RFR: 8332313: Update code review guidelines [v2]
On Thu, 16 May 2024 08:48:14 GMT, John Hendrikx wrote:
>> I agree with the concern, but I still think it's much better to encourage
>> developers to do formatting in a separate issue (or not at all) with all the
>> required administration, than to sneak in a formatting change in a PR that
>> has nothing to do with formatting. I prefer the noise to be completely
>> isolated, so that it can be ignored easily, rather than being distracted by
>> it in a PR.
>
> I think this conflicts a bit with the previous statement ("don't worry too
> much about import order"). In my experience, when merging complex changes,
> import order changes are often the cause of conflicts. We also found that
> with many different committer preferences, the whole import block could
> change from one commit to the next, which will lead to a conflict if such a
> file is part of a backport.
>
> We solved this by simply mandating a single order for imports (alphabetical,
> no gaps, no star imports, followed by separate block for static imports with
> the same rules) and made the build fail when it was violated. The number of
> conflicts went down dramatically. This does require a one time import fix of
> the code base, which will also be a source of conflicts, but at least it will
> be in the usual area (imports) and should solve the problem once and for all.
I removed the bit encouraging reformatting PRs (but left the "don't reformat
existing code..." bit.
As for the import order, this came up recently in Andy's PRs to remove
unnecessary imports. I'll file a follow-on issue to evaluate whether we want to
change our current policy.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1605067813
Re: RFR: 8332313: Update code review guidelines
On Thu, 16 May 2024 07:30:31 GMT, Johan Vos wrote:
>> Yeah, that was a typo (which I didn't notice when copying the block from the
>> other doc). I'll fix it. And I agree with your concern, so I'll remove the
>> last sentence.
>
> I agree with the concern, but I still think it's much better to encourage
> developers to do formatting in a separate issue (or not at all) with all the
> required administration, than to sneak in a formatting change in a PR that
> has nothing to do with formatting. I prefer the noise to be completely
> isolated, so that it can be ignored easily, rather than being distracted by
> it in a PR.
I think this conflicts a bit with the previous statement ("don't worry too much
about import order"). In my experience, when merging complex changes, import
order changes are often the cause of conflicts. We also found that with many
different committer preferences, the whole import block could change from one
commit to the next, which will lead to a conflict if such a file is part of a
backport.
We solved this by simply mandating a single order for imports (alphabetical, no
gaps, no star imports) and made the build fail when it was violated. The
number of conflicts went down dramatically. This does require a one time
import fix of the code base, which will also be a source of conflicts, but at
least it will be in the usual area (imports) and should solve the problem once
and for all.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602895839
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 21:57:23 GMT, Kevin Rushforth wrote: >> CONTRIBUTING.md line 233: >> >>> 231: * Don't worry too much about import order. Try not to change it but >>> don't worry about fighting your IDE to stop it from doing so. >>> 232: >>> 233: New code should be formatted consistently in accordance with the above >>> guidelines. However, please do not reformat existing code as part of a bug >>> fix. The makes more changes for code reviewers to track and review, and can >>> lead to merge conflicts. If you want to reformat a class, do that in a >>> separate pull request (which will need its own unique JBS bug ID). >> >> "The makes more changes" ? I think you mean "This" not "The" >> >> I'm not sure about the last sentence, it seems to encourage reformatting >> fixes which are just noise most of the time. > > Yeah, that was a typo (which I didn't notice when copying the block from the > other doc). I'll fix it. And I agree with your concern, so I'll remove the > last sentence. I agree with the concern, but I still think it's much better to encourage developers to do formatting in a separate issue (or not at all) with all the required administration, than to sneak in a formatting change in a PR that has nothing to do with formatting. I prefer the noise to be completely isolated, so that it can be ignored easily, rather than being distracted by it in a PR. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602767101
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 19:39:10 GMT, Kevin Rushforth wrote: >> 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. In the ideal world where we have tons of regression and compatibility tests, I would agree. Unfortunately, we are totally not there yet. Compared to other projects, the quality of tests in OpenJFX is good, but given the multitude of usage scenario's in client development, we would need much, much more tests before we can rely on them to detect regression. Therefore, the #1 point imho is that both the author (as Andy commented) and the reviewer have a real good understanding on what is happening. Every single change in a PR should be understood. The most dangerous things are "I don't understand why, but it seems to fail before and succeed after the patch". - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602762164
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 20:18:05 GMT, Phil Race wrote: >> This is out of scope for this PR, but it might be something to consider >> addressing in a follow-on enhancement. > > It should be made clear (somewhere, at some point) that it would be rare for > a CSR to be completed and approved before starting work on the implementation > because the spec. almost always evolves as the fix is developed. Yes, that is a good point. I'll file a follow-up to do that (since this section is otherwise unmodified from the current guidelines on the Wiki). - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602318735
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 20:08:05 GMT, John Hendrikx 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 79: > >> 77: * All Reviewers who have requested the chance to review have done so (or >> indicated that they are OK with it going in without their review). In rare >> cases a Project Lead may override this. >> 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business >> day (24 hours), not including weekends or major holidays. This is to allow >> sufficient time for those reviewers who might be in other time zones the >> chance to review if they have concerns. This is measured from the time that >> Skara has most recently added the "rfr" label (for example, for a PR that >> was previously in Draft mode, wait for at least 24 hours after the PR has >> been taken out of Draft and marked "rfr"). In rare cases (e.g., a build >> breakage) a Reviewer might give the OK to integrate without waiting for 24 >> hours. >> 79: * Verify the commit message. The Skara tooling adds a comment near the >> top of the PR with the commit message that will be used. You can add a >> summary to the commit message with the `/summary` command. You can add >> additional contributors with the `/contributor` command. Commands are issued >> by adding a comment to the PR that starts with a slash `/` character. > > The last sentence feels a bit out of place, as several places earlier in this > document already mention slash commands. I'll remove it (I moved it from the CONTRIBUTING doc, and it does seem out of place). Actually as someone else commented, Skara will verify that the PR title and JBS bug match, so mostly this is about adding a summary and/or contributors if you want to. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602315921
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 19:50:32 GMT, Andy Goryachev wrote: >> 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: >> >> I would add that it's a good idea to search for JBS issues similar/linked to >> the one being targeted. There are many unfound duplicates in JBS that arise >> only when searching, or at least closely linked ones (problems in >> `TableTreeView` often have "pseudo-duplicates" for `TreeView` and >> `TableView`, for example). >> >> I would explain (or link to where it is explained) when to close an issue a >> a duplicated vs. when to use the /add Skara command to bundle issues into >> the same fix. >> >> We might also find that the targeted issue is a regression of another issue. >> This is somewhat mentioned in the bullet point below "Consider the risk of >> regression". > > Second this, although I think the bulk of the work should fall on the author > of the PR. This would probably be better in the CONTRIBUTING guidelines (for PR authors). Maybe as a follow-on, but I'll give it some thought. >> 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. > ...inadvertent introduction of new API (that will have to be deprecated if > missed) I think this is worth mentioning. > I was wondering if there ought to be an unofficial checklist for things to > look for? That's sort of what this section is meant to be. As long as it doesn't get too verbose we could add additional things to look for. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602302685 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602306295
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 20:23:37 GMT, Nir Lisker wrote: >> Isn't this automatic? Seems weird you could integrate without these passing. > >> or is it? :-) > >  > > Looks to me like it is... A passing GHA test run is neither necessary nor sufficient. It is an interesting data point. A PR can be integrated with a failing run as long as we understand why it failed. It is something for a reviewer to check and ask about if they suspect it points to a real problem. I'll reword this a little bit to make it clear. I'm sort of glad that GitHub / Azure chose today to break it. It is a great illustration of why we don't want it to be blocking. Btw, I filed https://bugs.openjdk.org/browse/JDK-8332328 to track the GHA failure. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602309862
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 20:12:09 GMT, Phil Race wrote: >> 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). >> >> "but **It** is" -> it > > I think it worth noting that in skara syntax that isn't two people with the > reviewer role. > And tell people what to use if that is what they intend - eg if I have it > right It's `/reviewers 2 reviewers`, so I'll add that as an example and clarify that `/reviewers 2` is 2 with at least one reviewer. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602299262
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 19:04:19 GMT, Nir Lisker 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). >
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 20:33:37 GMT, Andy Goryachev wrote: >> He means people reading reviews are usually on github and your github ID is >> kevinrusforth, >> so they might be confused by "kcr" which is your openjdk id. >> >> Since the context is identifying OpenJDK Project leads, I think the OpenJDK >> ID is the right and only one to include here. At most you could clarify >> like (OpenJDK ID - kcr) > > exactly my point. when the majority of time is spent in the context of > github (PR review etc) it might be confusing when one types `@jvos` and it's > not found, or when within a PR there are references to both `jvos` and > `johanvos` > > (see the very first comment in https://github.com/openjdk/jfx/pull/1388 ) Ah, I misunderstood Andy's comment. Yes, I'll update this. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602297383
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 20:07:06 GMT, Phil Race 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 > > CONTRIBUTING.md line 233: > >> 231: * Don't worry too much about import order. Try not to change it but >> don't worry about fighting your IDE to stop it from doing so. >> 232: >> 233: New code should be formatted consistently in accordance with the above >> guidelines. However, please do not reformat existing code as part of a bug >> fix. The makes more changes for code reviewers to track and review, and can >> lead to merge conflicts. If you want to reformat a class, do that in a >> separate pull request (which will need its own unique JBS bug ID). > > "The makes more changes" ? I think you mean "This" not "The" > > I'm not sure about the last sentence, it seems to encourage reformatting > fixes which are just noise most of the time. Yeah, that was a typo (which I didn't notice when copying the block from the other doc). I'll fix it. And I agree with your concern, so I'll remove the last sentence. > README-code-reviews.md line 14: > >> 12: ### Reviewers >> 13: >> 14: The [List of Reviewers](https://openjdk.java.net/census#openjfx) is on >> the OpenJDK Census. > > We use ".org" now, not ".java.net" Yes, I missed this. I'll update. > README-code-reviews.md line 40: > >> 38: ### 1. The Reviewer role for the OpenJFX Project >> 39: >> 40: We define a formal "Reviewer" role, similar to the JDK project. A >> [Reviewer](https://openjdk.java.net/census#openjfx) is responsible for >> reviewing code changes and helping to determine whether a change is suitable >> for including into OpenJFX. We expect Reviewers to feel responsible not just >> for their piece, but for the quality of the JavaFX library as a whole. In >> other words, the role of Reviewer is one of stewardship. See the following >> section for what constitutes a good review. > > (https://openjdk.java.net/census#openjfx) > > .org please > > BTW these very long source lines make it awkward to precisely identify the > text I'm commenting on. I'll fix it. > README-code-reviews.md line 77: > >> 75: >> 76: * All substantive feedback has been addressed, especially any objections >> from one with a Reviewer role. >> 77: * All Reviewers who have requested the chance to review have done so (or >> indicated that they are OK with it going in without their review). In rare >> cases a Project Lead may override this. > > One thing to add here (or hereabouts) is that if someone has commented on > your review and requested changes that in almost all cases you should expect > that they will want to return to review the results. So DO NOT push without > letting earlier reviewers who made substantive comments re-review. I'll add something about this. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602296959 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602297934 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602298847 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602310947
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 Thanks for the additional feedback. I'll update the PR in the next day or two. I'll double-check that I addressed all of the comments. - PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2059087200
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 20:03:01 GMT, Phil Race wrote: >> Not sure what you mean, but it doesn't seem related to this PR. > > He means people reading reviews are usually on github and your github ID is > kevinrusforth, > so they might be confused by "kcr" which is your openjdk id. > > Since the context is identifying OpenJDK Project leads, I think the OpenJDK > ID is the right and only one to include here. At most you could clarify like > (OpenJDK ID - kcr) exactly my point. when the majority of time is spent in the context of github (PR review etc) it might be confusing when one types `@jvos` and it's not found, or when within a PR there are references to both `jvos` and `johanvos` (see the very first comment in https://github.com/openjdk/jfx/pull/1388 ) - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602212035
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 20:08:10 GMT, John Hendrikx wrote: >> or is it? :-) > > Isn't this automatic? Seems weird you could integrate without these passing. > or is it? :-)  Looks to me like it is... - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602201785
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 19:00:57 GMT, Nir Lisker 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). > > "but **It** is" -> it I think it worth noting that in skara syntax that isn't two people with the reviewer role. And tell people what to use if that is what they intend - eg if I have it right - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602190743
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 19:32:04 GMT, Kevin Rushforth wrote: >> 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. It should be made clear (somewhere, at some point) that it would be rare for a CSR to be completed and approved before starting work on the implementation because the spec. almost always evolves as the fix is developed. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602196255
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 CONTRIBUTING.md line 233: > 231: * Don't worry too much about import order. Try not to change it but > don't worry about fighting your IDE to stop it from doing so. > 232: > 233: New code should be formatted consistently in accordance with the above > guidelines. However, please do not reformat existing code as part of a bug > fix. The makes more changes for code reviewers to track and review, and can > lead to merge conflicts. If you want to reformat a class, do that in a > separate pull request (which will need its own unique JBS bug ID). "The makes more changes" ? I think you mean "This" not "The" I'm not sure about the last sentence, it seems to encourage reformatting fixes which are just noise most of the time. README-code-reviews.md line 14: > 12: ### Reviewers > 13: > 14: The [List of Reviewers](https://openjdk.java.net/census#openjfx) is on > the OpenJDK Census. We use ".org" now, not ".java.net" README-code-reviews.md line 40: > 38: ### 1. The Reviewer role for the OpenJFX Project > 39: > 40: We define a formal "Reviewer" role, similar to the JDK project. A > [Reviewer](https://openjdk.java.net/census#openjfx) is responsible for > reviewing code changes and helping to determine whether a change is suitable > for including into OpenJFX. We expect Reviewers to feel responsible not just > for their piece, but for the quality of the JavaFX library as a whole. In > other words, the role of Reviewer is one of stewardship. See the following > section for what constitutes a good review. (https://openjdk.java.net/census#openjfx) .org please BTW these very long source lines make it awkward to precisely identify the text I'm commenting on. README-code-reviews.md line 77: > 75: > 76: * All substantive feedback has been addressed, especially any objections > from one with a Reviewer role. > 77: * All Reviewers who have requested the chance to review have done so (or > indicated that they are OK with it going in without their review). In rare > cases a Project Lead may override this. One thing to add here (or hereabouts) is that if someone has commented on your review and requested changes that in almost all cases you should expect that they will want to return to review the results. So DO NOT push without letting earlier reviewers who made substantive comments re-review. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602185573 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602187001 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602188630 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602198401
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 Marked as reviewed by jhendrikx (Committer). README-code-reviews.md line 79: > 77: * All Reviewers who have requested the chance to review have done so (or > indicated that they are OK with it going in without their review). In rare > cases a Project Lead may override this. > 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business > day (24 hours), not including weekends or major holidays. This is to allow > sufficient time for those reviewers who might be in other time zones the > chance to review if they have concerns. This is measured from the time that > Skara has most recently added the "rfr" label (for example, for a PR that was > previously in Draft mode, wait for at least 24 hours after the PR has been > taken out of Draft and marked "rfr"). In rare cases (e.g., a build breakage) > a Reviewer might give the OK to integrate without waiting for 24 hours. > 79: * Verify the commit message. The Skara tooling adds a comment near the > top of the PR with the commit message that will be used. You can add a > summary to the commit message with the `/summary` command. You can add > additional contributors with the `/contributor` command. Commands are issued > by adding a comment to the PR that starts with a slash `/` character. The last sentence feels a bit out of place, as several places earlier in this document already mention slash commands. - PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2058908894 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602186633
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 19:53:43 GMT, Andy Goryachev wrote: >> 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? :-) Isn't this automatic? Seems weird you could integrate without these passing. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602186713
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 18:56:29 GMT, Kevin Rushforth wrote: >> README-code-reviews.md line 10: >> >>> 8: >>> 9: __Project Co-Lead__: Kevin Rushforth (kcr) >>> 10: __Project Co-Lead__: Johan Vos (jvos) >> >> There are two sets of ids - one for OpenJFX/JBS and one for Github. This >> might be confusing sometimes, should we list both? > > Not sure what you mean, but it doesn't seem related to this PR. He means people reading reviews are usually on github and your github ID is kevinrusforth, so they might be confused by "kcr" which is your openjdk id. Since the context is identifying OpenJDK Project leads, I think the OpenJDK ID is the right and only one to include here. At most you could clarify like (OpenJDK ID - kcr) - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602181574
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 19:23:25 GMT, Nir Lisker 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
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 19:16:31 GMT, Nir Lisker 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 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: > > I would add that it's a good idea to search for JBS issues similar/linked to > the one being targeted. There are many unfound duplicates in JBS that arise > only when searching, or at least closely linked ones (problems in > `TableTreeView` often have "pseudo-duplicates" for `TreeView` and > `TableView`, for example). > > I would explain (or link to where it is explained) when to close an issue a a > duplicated vs. when to use the /add Skara command to bundle issues into the > same fix. > > We might also find that the targeted issue is a regression of another issue. > This is somewhat mentioned in the bullet point below "Consider the risk of > regression". Second this, although I think the bulk of the work should fall on the author of the PR. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602168856
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 I replied to Andy's comments inline. Some of them are in unmodified sections and are out of scope for this, but I'll file a follow-on issue (P4). I'll address the rest of the feedback in a subsequent commit. - PR Review: https://git.openjdk.org/jfx/pull/1455#pullrequestreview-2058843801
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 18:20:22 GMT, Andy Goryachev 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
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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). "but **It** is" -> it 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. 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 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
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 18:05:10 GMT, Andy Goryachev 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 > > CONTRIBUTING.md line 52: > >> 50: >> 51: * File a bug in JBS for every pull request >> 52: > > for line 53, would it make more sense to use this link > https://bugs.openjdk.org/projects/JDK/issues/ > ? Whether or not that might be a better link, it's unrelated to this PR. The scope of this PR is limited to guidelines surrounding reviewing, integrating, and sponsoring PRs. > README-code-reviews.md line 10: > >> 8: >> 9: __Project Co-Lead__: Kevin Rushforth (kcr) >> 10: __Project Co-Lead__: Johan Vos (jvos) > > There are two sets of ids - one for OpenJFX/JBS and one for Github. This > might be confusing sometimes, should we list both? Not sure what you mean, but it doesn't seem related to this PR. - PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602112407 PR Review Comment: https://git.openjdk.org/jfx/pull/1455#discussion_r1602113564
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 CONTRIBUTING.md line 52: > 50: > 51: * File a bug in JBS for every pull request > 52: for line 53, would it make more sense to use this link https://bugs.openjdk.org/projects/JDK/issues/ ? README-code-reviews.md line 10: > 8: > 9: __Project Co-Lead__: Kevin Rushforth (kcr) > 10: __Project Co-Lead__: Johan Vos (jvos) There are two sets of ids - one for OpenJFX/JBS and one for Github. This might be confusing sometimes, should we list both? 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?). 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. 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? README-code-reviews.md line 78: > 76: * All substantive feedback has been addressed, especially any objections > from one with a Reviewer role. > 77: * All Reviewers who have requested the chance to review have done so (or > indicated that they are OK with it going in without their review). In rare > cases a Project Lead may override this. > 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business > day (24 hours), not including weekends / or major holidays. This is to allow > sufficient time for those reviewers who might be in other time zones the > chance to review if they have concerns. This is measured from the time that > Skara has most recently added the "rfr" label (for example, for a PR that was > previously in Draft mode, wait for at least 24 hours after the PR has been > taken out of Draft and marked "rfr"). In rare cases (e.g., a build breakage) > a Reviewer might give the OK to integrate without waiting for 24 hours. nit pick: 24 hours does not always equal to one business day. A long weekend may extend the waiting period to 96 hours, a "winter break" might span the whole week. README-code-reviews.md line 79: > 77: * All Reviewers who have requested the chance to review have done so (or > indicated that they are OK with it going in without their review). In rare > cases a Project Lead may override this. > 78: * The PR has been "rfr" (as indicated by Skara) for at least 1 business > day (24 hours), not including weekends / or major holidays. This is to allow > sufficient time for those reviewers who might be in other time zones the > chance to review if they have concerns. This is measured from the time that > Skara has most recently added the "rfr" label (for example, for a PR that was > previously in Draft mode, wait for at least 24 hours after the
Re: RFR: 8332313: Update code review guidelines
On Wed, 15 May 2024 17:45:56 GMT, Kevin Rushforth 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 Reviewers: @johanvos @andy-goryachev-oracle @arapte @prrace I'd like at least 3 "R"eviewers to review this. Committers and Authors are also welcome to provide feedback. - PR Comment: https://git.openjdk.org/jfx/pull/1455#issuecomment-2113119065
RFR: 8332313: Update code review guidelines
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 - Commit messages: - Update CONTRIBUTING.md and README-code-reviews.md - 8332313: Update code review guidelines - Convert Code Review Policies Wiki to README-code-reviews.md Changes: https://git.openjdk.org/jfx/pull/1455/files Webrev: https://webrevs.openjdk.org/?repo=jfx=1455=00 Issue: https://bugs.openjdk.org/browse/JDK-8332313 Stats: 115 lines in 2 files changed: 106 ins; 0 del; 9 mod Patch: https://git.openjdk.org/jfx/pull/1455.diff Fetch: git fetch https://git.openjdk.org/jfx.git pull/1455/head:pull/1455 PR: https://git.openjdk.org/jfx/pull/1455
