On Sun, 24 Aug 2025 19:52:48 GMT, John Hendrikx <[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 ... > > 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`? I was thinking about these `RuntimeException`s, but didn't want to touch it. Since IAE is a subtype, it should be fine to replace. > 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... Fixed ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2297627981 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2297634922
