On Wed, 15 May 2024 19:50:32 GMT, Andy Goryachev <ango...@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
