On Fri, 8 Sep 2023 15:01:34 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> In the Control hierarchy, all property accessor methods must be declared
>> `final`.
>>
>> Added a test to check for missing `final` keyword and added the said keyword
>> where required.
>
> Andy Goryachev has updated the pull request incrementally with one additional
> commit since the last revision:
>
> only public
Looks good. Added some minor comments.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 99:
> 97: public class ControlPropertiesTest {
> 98:
> 99: private static final boolean FAIL_FAST = true;
Please add a short comment on what this is for. Like "If true, an exception
will be thrown on encountering the first violating class, otherwise a list of
all violating cases will be printed".
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 102:
> 100:
> 101: // list all current descendants of Control class.
> 102: private Set<Class> allControlClasses() {
Please use `Class<?>` instead of raw types. In 2 other places below as well.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 103:
> 101: // list all current descendants of Control class.
> 102: private Set<Class> allControlClasses() {
> 103: return Set.of(
Not that it matters much since the method is called once, but why is there a
need for a method that returns the same constant set? The set can just be a
constant: `private final Set<Class<?>> = Set.of(...);`
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 216:
> 214: } else {
> 215: System.err.println(msg);
> 216: }
No need for an `else` clause since `throw` will exit the scope of the method if
it's reached.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1213#pullrequestreview-1618032226
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1320200154
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1320199287
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1320196311
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1320197915
