Re: RFR: 8332313: Update code review guidelines [v2]

2024-05-21 Thread Kevin Rushforth
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]

2024-05-18 Thread Nir Lisker
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]

2024-05-18 Thread Kevin Rushforth
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]

2024-05-17 Thread Nir Lisker
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]

2024-05-17 Thread Nir Lisker
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]

2024-05-17 Thread Kevin Rushforth
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]

2024-05-17 Thread Kevin Rushforth
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]

2024-05-17 Thread Kevin Rushforth
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]

2024-05-17 Thread Kevin Rushforth
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]

2024-05-17 Thread Kevin Rushforth
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]

2024-05-17 Thread Kevin Rushforth
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]

2024-05-17 Thread Kevin Rushforth
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]

2024-05-17 Thread Kevin Rushforth
On Wed, 15 May 2024 22:13:34 GMT, Kevin Rushforth  wrote:

>>> or is it? :-)
>> 
>> ![image](https://github.com/openjdk/jfx/assets/37422899/8daab7cf-f050-4964-b8a6-731666422293)
>> 
>> 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]

2024-05-17 Thread Kevin Rushforth
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]

2024-05-17 Thread Kevin Rushforth
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]

2024-05-17 Thread Kevin Rushforth
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]

2024-05-17 Thread Kevin Rushforth
> 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&pr=1455&range=01
 - incr: https://webrevs.openjdk.org/?repo=jfx&pr=1455&range=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]

2024-05-17 Thread Kevin Rushforth
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