On Fri, 18 Aug 2023 22:17:15 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
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 84:
> 82: import javafx.scene.control.cell.TextFieldTreeTableCell;
> 83: import org.junit.Assert;
> 84: import org.junit.Test;
These are JUnit 4, we should use 5 for new tests.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 98:
> 96: */
> 97: public class ControlPropertiesTest {
> 98: private static final boolean FAIL_FAST = true;
We use an empty line after the class declaration.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 103:
> 101: //
> https://stackoverflow.com/questions/28678026/how-can-i-get-all-class-files-in-a-specific-package-in-java
> 102: private Class[] allControlClasses() {
> 103: return new Class[] {
I was also thinking that these should be generated by the contents of the
package. When I looked at javafx.scene.control I saw classes that weren't
controls, but were used by them, so they would need to be removed. So it's a
question of whitelist vs. blacklist.
In any case, I'm not sure why you're using an array here. `Set.of` seems more
suitable and will also check for duplicate entries.
modules/javafx.controls/src/test/java/test/javafx/scene/control/ControlPropertiesTest.java
line 232:
> 230: private String err(Method method, String message) {
> 231: return method + " " + message;
> 232: }
(This comment didn't appear in the review for some reason.)
I think that the code could be a bit more clean. I took a stab at it:
Set<Class<?>> CLASSES = Set.of(/*...*/);
private void check(Class<?> cls) {
Map<String, Method> methods =
Stream.of(cls.getMethods()).collect(Collectors.toMap(Method::getName, m -> m));
// the loop is also fine
for (var method : methods.values()) {
String name = method.getName();
if (name.endsWith("Property")) {
checkModifiers(method);
String propName = name.substring(0, name.length() -
"Property".length());
check(methods, propName, "get", 0);
check(methods, propName, "set", 1);
check(methods, propName, "is", 0);
}
}
}
private void check(Map<String, Method> methods, String name, String prefix,
int numArgs) {
String methodName = new StringJoiner("", prefix, name.substring(0,
1).toUpperCase() + name.substring(1)).toString();
Method m = methods.get(methodName);
if (m != null && m.getParameterCount() == numArgs) {
checkModifiers(m);
}
}
private void checkModifiers(Method m) {
int mod = m.getModifiers();
if (Modifier.isPublic(mod) && !Modifier.isFinal(mod)) {
if (FAIL_FAST) {
throw new AssertionError(errorMessage(m));
} else {
System.err.println(errorMessage(m));
}
}
}
private String errorMessage(Method method) {
return method + "is not final";
}
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1299310658
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1299310698
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1299311517
PR Review Comment: https://git.openjdk.org/jfx/pull/1213#discussion_r1299436111
