On Thu, 4 Feb 2021 20:01:10 GMT, Nir Lisker <nlis...@openjdk.org> 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 added test will look meaningless after the patch, since it's trying to force a change that can't happen anymore, and this can be confusing in the future. Does it still make sense to commit it? It's more of a demonstration of the bug and the fix. I also noticed that other tests in `LocalDateTimeStringConverterTest` are not named with `test` in the beginning. Is this wrong? ------------- PR: https://git.openjdk.java.net/jfx/pull/393
