On Sun, 24 Aug 2025 20:41:45 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Refactoring of all `StringConverter`s and their tests. General notes: >> * The documentation language has been unified and `null` parameter rules >> have been documented. >> * Tests have been cleaned up in the vein of >> https://github.com/openjdk/jfx/pull/1759 and unneeded `@BeforeAll`s were >> removed. >> * Internal fields were made `private final` to guarantee immutability. >> >> Incremental commits are provided for easier reviewing: >> >> ### Parent classes >> * `StringConverter`: updated documentation >> * `BaseStringConverter`: a new internal class that implements repeated code >> from converter implementations and serves as an intermediate superclass. It >> does empty and `null` string checks that are handled uniformly, except for >> `DefaultStringConverter`, which has a different formatting mechanism. >> >> ### Primitive-related converters >> * All primitive (wrapper) converters also document their formatting and >> parsing mechanism since these are "well-established". >> >> ### Format converter >> * Checked for `null` during constriction time to avoid runtime NPEs. >> * There is no test class for this converter. A followup might be desirable. >> * A followup should deprecate for removal `protected Format getFormat()` (as >> in [JDK-8314597](https://bugs.openjdk.org/browse/JDK-8314597) and >> [JDK-8260475](https://bugs.openjdk.org/browse/JDK-8260475). >> >> ### Number and subclasses converters >> * The intermediate `locale` and `pattern` fields were removed (along with >> their tests). The class generated a new formatter from these on each call. >> This only makes sense for mutable fields where the resulting formatter can >> change, but here the formatter can be computed once on construction and >> stored. >> * The only difference between these classes is a single method for creating >> a format from a `null` pattern, which was encapsulated in the >> `getSpecializedNumberFormat` method. >> * The terminally deprecated `protected NumberFormat getNumberFormat()` was >> removed. Can be split to its own issue if preferred. In my opinion, it >> shouldn't exist even internally since testing the internal formatter doesn't >> help. The only tests here should be for to/from strings, and these are >> lacking. A followup can be filed for adding more conversion tests. >> >> ### Date/Time converters >> * Added a documentation note advising users to use the `java.time` classes >> instead of the old `Date` class. >> * As with Number converters, only the `dateFormat` field was kept, which is >> created once on construction instead of on each ... > > modules/javafx.base/src/main/java/javafx/util/converter/LocalDateTimeStringConverter.java > line 82: > >> 80: /// [IsoChronology#INSTANCE] will be used. >> 81: public LocalDateTimeStringConverter(FormatStyle dateStyle, >> FormatStyle timeStyle, Locale locale, Chronology chronology) { >> 82: // JEP-513 could make this look better by moving the null checks >> before super > > This seems like an irrelevant comment. I doubt it would even look better if > you did this (as you'd require variables). How about just making it nice to > read like: > > > super( > Objects.requireNonNullElse(dateStyle, FormatStyle.SHORT), > Objects.requireNonNullElse(timeStyle, FormatStyle.SHORT), > locale, > chronology > ); > > > Also, you're using here an indent that is not a multiple of 4. I think that the checks before calling the constructors make code clearer. I've been using flexible constructor bodies for this for a couple of versions now and I prefer it. As for the indent, it aligns with the parameter above it. > modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateStringConverterTest.java > line 72: > >> 70: NO_PARAM, >> 71: WITH_FORMATTER_PARSER, >> 72: WITH_FORMAT_STYLES, > > I know enums allow it, but trailing comma is still ugly I think that in JavaFX we use a `;`. The comma stayed from the old code, but I fixed it. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2312083596 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2312084263
