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 [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 [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
