On Mon, 21 Aug 2023 23:11:49 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:
>
> review comments
Looks good. Gave a couple of minor optional suggestions.
As to the topic of class finding, I think it's fine to use a manual list, at
least for now. The complexity of automatically detecting the classes might not
be worth the hassle. Leaving it up to you.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 91:
> 89:
> 90: /**
> 91: * Tests contract for properties and their accessors in the Control type
> hierarchy.
Mutators too?
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 168:
> 166:
> 167: /**
> 168: * Tests for missing final keyword in Control properties and their
> accessors.
Technically, we're looking at mutators too, not just accessors.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 172:
> 170: @Test
> 171: public void testMissingFinalMethods() {
> 172: for (Class c: allControlClasses()) {
I think we use a space before `:`. Don't know if it's being enforced. If you
change it there are 2 more places.
-------------
Marked as reviewed by nlisker (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1213#pullrequestreview-1588935327
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1301317702
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1301317162
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1301316459
