RFR: 8332313: Update code review guidelines

[Kevin Rushforth]

On Wed, 15 May 2024 19:50:32 GMT, Andy Goryachev <angorya at openjdk.org> 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