8250802: Refactor StringConverter and its subclasses

[nlisker]

Refactoring of all StringConverters 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 8340464: [TestBug] Convert parametrized base tests to JUnit 5 #1759 and unneeded @BeforeAlls 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 and 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 call.
• As with Number converters, the getSpecialziedDateFormat method is the only difference between the classes.
• As with Number converters, protected DateFormat getDateFormat() has been removed from the subclasses and shouldn't exist internally either in my opinion.

LocalDate/Time converters

• The structure of these classes is different from the above 2 groups since there is no subclassing. Instead, the Date and Time converters used the DateTime converter's LdtConverter implementation by passing their class type, which is not a good design. Instead, an internal superclass BaseTemporalStringConverter<T extends Temporal> was introduced along with its shim.
• The code that fixes the 4 year pattern, fixFourDigitYear, is now checked before creating a formatter, avoiding unneeded formatter creations.
• As above, only the parser and formatter fields were kept.
• As above, the getLocalizedFormatter and getTemporalQuery() methods are the only differences between the subclasses.
• As above, testing the formatter and parser isn't useful in my opinion.

/reviewers 2
/csr

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change requires a CSR request matching fixVersion jfx26 to be approved (needs to be created)
• Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

• JDK-8250802: Refactor StringConverter and its subclasses (Enhancement - P4)

Reviewers

• John Hendrikx (@hjohn - Reviewer) 🔄 Re-review required (review applies to 2b610c6a)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1880/head:pull/1880
$ git checkout pull/1880

Update a local copy of the PR:
$ git checkout pull/1880
$ git pull https://git.openjdk.org/jfx.git pull/1880/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1880

View PR using the GUI difftool:
$ git pr show -t 1880

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1880.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back nlisker! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[openjdk]

@nlisker
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

[openjdk]

@nlisker has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@nlisker please create a CSR request for issue JDK-8250802 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[mlbridge]

Webrevs

• 01: Full - Incremental (3448438b)
• 00: Full (2b610c6a)

[hjohn]

Looks fine, left some readability/style comments

[hjohn]

Shouldn't that be FormatStringConverter that it creates?

To be honest, I never bother repeating this on constructors, so I just write "Creates a new instance ... "

[nlisker]

All the converters used this language prior. The language on constructors isn't unified across JavaFX and the JDK. Between "Creates a new default instance", "Creates a default ", "Constructs a..." and others I don't have a preference. I can replace all class names with "new instance" if you think it's better.

[hjohn]

super() here is just noise

[nlisker]

I opted to explicitly use super in these places for uniformity with the other constructors as it can help understand which constructor is called from where. The "constructor jungle" in these classes made me do things I don't normally do. In LocalDateStringConverter, for example, the constructor goes through a this constructor instead of super, so it can be confusing. Can remove anyway.

[hjohn]

Parameter reassignment is really bad form (Eclipse has a warning for it if you'd care to enable it).

Also I'm not sure I like Objects.requireNonNullElse; it's actually longer than:

this.locale = locale == null ? Locale.getDefault() : locale;

[nlisker]

Parameter reassignment is helpful when immediately "curating", e.g., substituting nulls and clamping (Java doesn't have the language mechanisms for dealing with this problem). It avoids using the wrong value later on by not having access to an illegal variable, and the parameter name still carries the same semantics.

I wasn't sure about Objects.requireNonNullElse. I used the ternary in other places where I didn't want the alternative result to be evaluate (the parallel of Objects.requireNonNullElseGet). The number of characters doesn't bother me because the method conveys a simple meaning, unlike the ternary where you actually need to figure out what each branch does. Some people import Objects statically since these null checks are very common.
I'll switch it anyway.

[hjohn]

This seems like an irrelevant comment. I doubt it would even look better if you did this (as you'd require variables). How about just making it nice to read like:

        super(
            Objects.requireNonNullElse(dateStyle, FormatStyle.SHORT),
            Objects.requireNonNullElse(timeStyle, FormatStyle.SHORT), 
            locale, 
            chronology
        );

Also, you're using here an indent that is not a multiple of 4.

[nlisker]

I think that the checks before calling the constructors make code clearer. I've been using flexible constructor bodies for this for a couple of versions now and I prefer it. As for the indent, it aligns with the parameter above it.