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 . I left one comment inline. I will likely have more minor comments, but this is shaping up nicely. The changes to not cache `Local.default()` look good. As do the restored specializations of the `toString` and `fromString` methods in the various subclasses. Speaking of which: > > I don't think so. Once you restore the method, javac will generate the same > > code with the updated PR as it does today with master. Any attempt to > > remove the specialized version of the methods in the future will run into > > the same problem. > > This is a general issue with Java generics: once you surface API with a > > more-specific parameterized type override, you are stuck with it. Same > > thing for an override with a covariant return type. > > I'm not sure how this is different than any other deprecation that causes a > source or binary incompatibility. Perhaps you're saying that the breakage > here is too large to justify the deprecation? When we deprecate a method for removal it is because we don't want applications to call that method. Either because we are removing some bit of functionality (very rarely would we do this) or because the method exposes functionality that was unintended or has consequences we prefer not to propagate. Neither of these apply here. If we were to terminally deprecate and then remove the overridden methods, applications would need to cast their instance of, say, `LocalDateTimeStringConverter` to `StringConverter<DateTime>` before calling `toString` or `fromString`. I don't see any benefit to doing that. ------------- PR Review: https://git.openjdk.org/jfx/pull/1880#pullrequestreview-4821296260
