On Thu, 30 Jul 2026 02:00:51 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 construction 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 three additional > commits since the last revision: > > - Update copyright year > - Moved implNote to method docs > - Remove unneeded . modules/javafx.base/src/main/java/javafx/util/StringConverter.java line 35: > 33: /// > 34: /// @implNote JavaFX's implementations are immutable (the same converter > can be reused, except for > 35: /// `DateTimeStringConverter`s that can only be reused on the same > thread). Implementing classes are not required to be Can you add a similar warning to `DateTimeStringConverter` please? modules/javafx.base/src/main/java/javafx/util/converter/BaseStringConverter.java line 51: > 49: abstract T fromNonEmptyString(String string); > 50: > 51: /// {@inheritDoc} `null` is converted to an empty string, otherwise > the type's `toString` is used. I don't think the javadoc in this class is being processed; we probably need to copy the text to all the descendants or the base class: <img width="381" height="695" alt="Image" src="https://github.com/user-attachments/assets/83fb71c1-5f90-48d5-8643-cda48d495ac1"; /> ------------- PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r3683715827 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r3683813124
