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
