On Wed, 19 Nov 2025 00:59:19 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 with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains 11 additional commits since > the last revision: > > - Merge master > - Fix typo > - Removed super() invocations > - review comments 2 > - review comments 1 > - LocalDate/Time converters > - Date/Time converters > - Number and subclasses converters > - Format converter > - Primitive-related converters > - ... and 1 more: https://git.openjdk.org/jfx/compare/f75fa093...a030ce58 modules/javafx.base/src/main/java/javafx/util/converter/CurrencyStringConverter.java line 37: > 35: > 36: /// Creates a `CurrencyStringConverter` that uses a formatter/parser > based on the user's [Locale]. > 37: public CurrencyStringConverter() {} very, very minor: I wish you wouldn't do `{}` - if anything needs to be inserted, it widens the diff. ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2543067295
