RFR: 8332313: Update code review guidelines [v2]

[Kevin Rushforth]

On Fri, 17 May 2024 15:02:14 GMT, Nir Lisker <nlisker at openjdk.org> 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