On Thu, 4 Feb 2021 20:01:10 GMT, Nir Lisker <[email protected]> wrote:
> Fixes a mutability issue for `LocalDateTimeStringConverter` (and
> `LocalDateStringConverter`) where the chronology can change during the
> lifetime of the instance and cause an inconsistent state. The following
> changes were made:
>
> * The input is now formatted and parsed directly with the formatter and
> parser (which are configured with a chronology) without the chronology field
> value of the class.
> * The chronology field value does not change anymore when there is an
> exception due to inability to format the date.
> * Logging on failed formatting was removed as it did not seem useful. The
> formatter will throw the same exception that the chronology field does
> anyway, so there is not much use to catching the exception before hand.
>
> Added a test that fails without this patch. The test creates a converter with
> an explicit `Chronology` and `null` parser and formatter. The default
> formatter that is created with the given chronology formats a legal date
> (with respect to the chronology) to a string, which the parser should be able
> to convert back to a date. However, by forcing an exception in the formatting
> process using an illegal date, the chronology changes, and now when the
> parser is used it is configured with the new chronology and it can't parse
> the string correctly.
The fix looks good to me, as does the test with a couple suggestions.
modules/javafx.base/src/test/java/test/javafx/util/converter/LocalDateTimeStringConverterTest.java
line 167:
> 165: // force a chronology change with an invalid Japanese date
> 166: try {
> 167: System.out.println(converter.toString(LocalDateTime.of(1, 1,
> 1, 1, 1, 1)));
Since you are expecting an exception, it seems a good idea to fail if you
didn't get one. Something like:
fail("Did not get the expected exception");
Also, you might consider narrowing the `catch` to just the exception that you
expect, so that an unexpected exception will also fail the test.
-------------
PR: https://git.openjdk.java.net/jfx/pull/393
