On Sat, 30 Aug 2025 20:11:36 GMT, Nir Lisker <[email protected]> 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... > > Nir Lisker has updated the pull request incrementally with two additional > commits since the last revision: > > - review comments 2 > - review comments 1 I think the change makes sense, despite a somewhat large footprint. Requesting the following changes: - sync up with the latest master - revert javadoc markdown modules/javafx.base/src/main/java/javafx/util/StringConverter.java line 38: > 36: /// - Except for `DefaultStringConverter`, formatting `null` returns an > empty string, otherwise the type's `toString` is > 37: /// used if it is suitable; parsing `null` or an empty string returns > `null`. > 38: /// - Immutable (the same converter can be reused). Is this true? I recall some formatters having a state, but I don't remember which... modules/javafx.base/src/main/java/javafx/util/converter/BaseStringConverter.java line 30: > 28: import javafx.util.StringConverter; > 29: > 30: /// A base class containing common implementations for `StringConverter`s > as noted in the @implNote of `StringConverter`. In light of possible backporting, it's been decided earlier not to use markdown comments. modules/javafx.base/src/main/java/javafx/util/converter/DateStringConverter.java line 99: > 97: > 98: @Override > 99: DateFormat getSpecialziedDateFormat(int dateStyle, int timeStyle, > Locale locale) { spelling modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 115: > 113: getSpecialziedDateFormat(dateStyle, timeStyle, locale) : > 114: new SimpleDateFormat(pattern, locale); > 115: dateFormat.setLenient(false); Would setting `lenient=false` constitute a change in functionality? modules/javafx.base/src/main/java/javafx/util/converter/TimeStringConverter.java line 101: > 99: > 100: @Override > 101: DateFormat getSpecialziedDateFormat(int dateStyle, int timeStyle, > Locale locale) { spelling modules/javafx.base/src/test/java/test/javafx/util/converter/CurrencyStringConverterTest.java line 58: > 56: void testConstructor_locale() { > 57: NumberFormat numberFormat = > NumberFormat.getCurrencyInstance(Locale.CANADA); > 58: very minor: extra newlines? modules/javafx.base/src/test/java/test/javafx/util/converter/NumberStringConverterTest.java line 58: > 56: void testConstructor_locale() { > 57: NumberFormat numberFormat = > NumberFormat.getNumberInstance(Locale.CANADA); > 58: minor: weird extra newlines ------------- Changes requested by angorya (Reviewer). PR Review: https://git.openjdk.org/jfx/pull/1880#pullrequestreview-3474802047 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535762634 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535714830 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535737237 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535744617 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535760564 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535779020 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535796857
