On Sun, 24 Aug 2025 10:13:25 GMT, Nir Lisker <nlis...@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 call. > * As with Number converters, the `getSpecialzied... Looks fine, left some readability/style comments modules/javafx.base/src/main/java/javafx/util/converter/BaseTemporalStringConverter.java line 50: > 48: protected BaseTemporalStringConverter(FormatStyle dateStyle, > FormatStyle timeStyle, Locale locale, Chronology chrono) { > 49: locale = Objects.requireNonNullElse(locale, DEFAULT_LOCALE); > 50: chrono = Objects.requireNonNullElse(chrono, DEFAULT_CHRONO); Parameter reassignment here. It all looks like fields, but only the last two are. Fields could be prefixed with `this.` to make this clearer, but IMHO parameter assignment is always bad. modules/javafx.base/src/main/java/javafx/util/converter/BaseTemporalStringConverter.java line 68: > 66: .toFormatter() > 67: .withChronology(chrono) > 68: > .withDecimalStyle(DecimalStyle.of(locale)); This is IMHO a bad way to indent things; not only is the indent no longer a multiple of 4, it also makes diffs larger than they need to be when say `DateTimeFormatterBuilder` is renamed or put in a variable. modules/javafx.base/src/main/java/javafx/util/converter/BaseTemporalStringConverter.java line 86: > 84: .withChronology(chrono) > 85: .withDecimalStyle(DecimalStyle.of(locale)); > 86: } Weird level of indent, perhaps use 4 or 8? modules/javafx.base/src/main/java/javafx/util/converter/CurrencyStringConverter.java line 39: > 37: public CurrencyStringConverter() { > 38: super(); > 39: } `super()` here is just noise modules/javafx.base/src/main/java/javafx/util/converter/DateStringConverter.java line 43: > 41: /// and the user's [Locale]. > 42: public DateStringConverter() { > 43: super(); `super()` here is just noise modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 99: > 97: /// both date and time styles. > 98: public DateTimeStringConverter(Locale locale, String pattern) { > 99: dateFormat = create(locale, DateFormat.DEFAULT, > DateFormat.DEFAULT, pattern); Just noting inconsistent style here (see below `this.dateFormat = ` or `dateFormat = `) -- I personally favor prefixing with `this.` when it is a write action. modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 112: > 110: > 111: private DateFormat create(Locale locale, int dateStyle, int > timeStyle, String pattern) { > 112: locale = Objects.requireNonNullElse(locale, DEFAULT_LOCALE); See previous comment about using `Objects.requireNonNullElse` and doing parameter reassignment. modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 119: > 117: } > 118: > 119: // treat as protected Seeing this a lot, what do you want to convey here? I'm just wondering what this would mean to a future maintainer... "don't accidentally make it protected because then it would be API"? Can add that to a lot of methods in FX then... If you really want to treat it as protected, then I expect a full javadoc. If you just want an inbetween class with helpful methods that you don't want to make public, then making the class and all its methods package private is a standard pattern (see StringBuilder / StringBuffer)... So I'm just wondering what a future maintainer will use this comment for? modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 129: > 127: return dateFormat.parse(string); > 128: } catch (ParseException e) { > 129: throw new RuntimeException(e); Should be `IllegalArgumentException` modules/javafx.base/src/main/java/javafx/util/converter/FormatStringConverter.java line 42: > 40: private final Format format; > 41: > 42: /// Creates a `StringConverter` for arbitrary types that uses the > given `Format`. Shouldn't that be `FormatStringConverter` that it creates? To be honest, I never bother repeating this on constructors, so I just write "Creates a new instance ... " modules/javafx.base/src/main/java/javafx/util/converter/FormatStringConverter.java line 57: > 55: T result = (T) format.parseObject(string, pos); > 56: if (pos.getIndex() != string.length()) { > 57: throw new RuntimeException("Parsed string not according to > the format"); Throw `IllegalArgumentException`? modules/javafx.base/src/main/java/javafx/util/converter/FormatStringConverter.java line 67: > 65: } > 66: > 67: /// {@return the `Format` instance for formatting and parsing in this > `StringConverter`} `FormatStringConverter` ? Or just leave that part of... 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. modules/javafx.base/src/main/java/javafx/util/converter/NumberStringConverter.java line 82: > 80: > 81: private NumberFormat createFormat(Locale locale, String pattern) { > 82: locale = Objects.requireNonNullElse(locale, Locale.getDefault()); Parameter reassignment is really bad form (Eclipse has a warning for it if you'd care to enable it). Also I'm not sure I like `Objects.requireNonNullElse`; it's actually longer than: this.locale = locale == null ? Locale.getDefault() : locale; modules/javafx.base/src/main/java/javafx/util/converter/NumberStringConverter.java line 97: > 95: return numberFormat.parse(string); > 96: } catch (ParseException e) { > 97: throw new RuntimeException(e); This really should be `IllegalArgumentException`. modules/javafx.base/src/main/java/javafx/util/converter/NumberStringConverter.java line 106: > 104: } > 105: > 106: /// {@return the `NumberFormat` used for formatting and parsing in > this `NumberStringConverter`} Is this even a valid comment with the return tag embedded in curly braces? It's also internal, so can remove. modules/javafx.base/src/main/java/javafx/util/converter/PercentageStringConverter.java line 38: > 36: /// Creates a `PercentageStringConverter` that uses a > formatter/parser based on the user's [Locale]. > 37: public PercentageStringConverter() { > 38: super(); `super()` here is just noise modules/javafx.base/src/main/java/javafx/util/converter/TimeStringConverter.java line 2: > 1: /* > 2: // * Copyright (c) 2010, 2024, Oracle and/or its affiliates. All rights > reserved. This looks like it shouldn't have been committed. modules/javafx.base/src/main/java/javafx/util/converter/TimeStringConverter.java line 43: > 41: /// time style, and the user's {@link Locale}. > 42: public TimeStringConverter() { > 43: super(); `super()` is just noise here modules/javafx.base/src/test/java/test/javafx/util/converter/DateStringConverterTest.java line 48: > 46: public class DateStringConverterTest { > 47: > 48: private static final Locale DEFALUT_LOCALE = > Locale.getDefault(Locale.Category.FORMAT); Typo: `DEFAULT_LOCALE` modules/javafx.base/src/test/java/test/javafx/util/converter/DateStringConverterTest.java line 75: > 73: new TestCase(new > DateStringConverter(DateFormat.getDateInstance(DateFormat.LONG)), > 74: DEFALUT_LOCALE, DateFormat.DEFAULT, null, > DateFormat.getDateInstance(DateFormat.LONG)) > 75: ); Wouldn't this look better: return List.of( new TestCase(new DateStringConverter(), DEFALUT_LOCALE, DateFormat.DEFAULT, null, null), new TestCase(new DateStringConverter(DateFormat.SHORT), DEFALUT_LOCALE, DateFormat.SHORT, null, null), new TestCase(new DateStringConverter(Locale.UK), Locale.UK, DateFormat.DEFAULT, null, null), new TestCase(new DateStringConverter(Locale.UK, DateFormat.SHORT), Locale.UK, DateFormat.SHORT, null, null), new TestCase(new DateStringConverter("dd MM yyyy"), DEFALUT_LOCALE, DateFormat.DEFAULT, "dd MM yyyy", null), new TestCase( new DateStringConverter(DateFormat.getDateInstance(DateFormat.LONG)), DEFALUT_LOCALE, DateFormat.DEFAULT, null, DateFormat.getDateInstance(DateFormat.LONG) ) ); modules/javafx.base/src/test/java/test/javafx/util/converter/DateTimeStringConverterTest.java line 73: > 71: new TestCase(new DateTimeStringConverter(), > 72: Locale.getDefault(Locale.Category.FORMAT), > DateFormat.DEFAULT, DateFormat.DEFAULT, > 73: null, null, VALID_DATE_WITH_SECONDS), Why not put the last closing bracket on a new line aligned with `new` just like everyone always does with the curly braces? Much more readable, and doesn't require the empty line between blocks to "make" it readable. So: return List.of( new TestCase(new DateTimeStringConverter(), Locale.getDefault(Locale.Category.FORMAT), DateFormat.DEFAULT, DateFormat.DEFAULT, null, null, VALID_DATE_WITH_SECONDS ), // add more here... ); modules/javafx.base/src/test/java/test/javafx/util/converter/DateTimeStringConverterTest.java line 94: > 92: Locale.getDefault(Locale.Category.FORMAT), > DateFormat.DEFAULT, DateFormat.DEFAULT, > 93: null, > DateFormat.getDateTimeInstance(DateFormat.LONG, DateFormat.FULL), > VALID_DATE_WITH_SECONDS) > 94: ); This makes it looks like it is the closing bracket related to `new TestCase` for example, while it is from the `List.of` 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 modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java line 78: > 76: NO_PARAM, > 77: WITH_FORMATTER_PARSER, > 78: WITH_FORMAT_STYLES, Trailing comma modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java line 134: > 132: case WITH_FORMATTER_PARSER -> new > LocalDateTimeStringConverter(aFormatter, aParser); > 133: case WITH_FORMAT_STYLES -> new > LocalDateTimeStringConverter(FormatStyle.SHORT, FormatStyle.SHORT, > 134: Locale.UK, IsoChronology.INSTANCE); Odd indent level, 4 or 8? modules/javafx.base/src/test/java/test/javafx/util/converter/LocalTimeStringConverterTest.java line 72: > 70: NO_PARAM, > 71: WITH_FORMATTER_PARSER, > 72: WITH_FORMAT_STYLES, Trailing comma modules/javafx.base/src/test/java/test/javafx/util/converter/NumberStringConverterTest.java line 44: > 42: private static final String PATTERN = "#,##,###,####"; > 43: > 44: private final NumberStringConverter usLocaleConverter = new > NumberStringConverter(Locale.US); can be `static` ? modules/javafx.base/src/test/java/test/javafx/util/converter/NumberStringConverterTest.java line 50: > 48: NumberFormat numberFormat = > NumberFormat.getNumberInstance(Locale.getDefault()); > 49: > 50: var nsc = new NumberStringConverter(); Why not `var converter`, `var c` or even just inlining this? I think these abbreviations are not helpful. This will make the tests all much more similar to each other. modules/javafx.base/src/test/java/test/javafx/util/converter/TimeStringConverterTest.java line 86: > 84: Locale.getDefault(Locale.Category.FORMAT), > DateFormat.DEFAULT, null, > 85: DateFormat.getTimeInstance(DateFormat.FULL), > VALID_TIME_WITH_SECONDS) > 86: ); See previous comment, this bracket seems related to `new TestCase`. ------------- Marked as reviewed by jhendrikx (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1880#pullrequestreview-3149487568 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296798578 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296799109 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296799512 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296786737 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296791683 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296792639 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296792949 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296793928 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296794534 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296785487 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296786007 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296786266 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296800138 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296787950 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296789162 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296789669 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296789894 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296794828 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296795007 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296796717 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296796208 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296797782 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296798081 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296801685 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296801878 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296802012 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296802251 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296791382 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296791086 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2296798294
