<i18n dev> RFR: 8154520: java.time: appendLocalizedOffset() should return the localized "GMT" string
thejasvi.v.voniadka at oracle.com
Wed Jul 3 12:32:29 UTC 2019
Thank you for the review. Please find the updated webrev:
The TCKOffsetPrinterParser.java has been reverted back to what it was, except for the copyright year and the locale addition. I have incorporated your comments to TestLocalizedOffsetPrinterParser.java.
From: Naoto Sato
Sent: Tuesday, July 02, 2019 9:33 PM
To: Thejasvi Voniadka <thejasvi.v.voniadka at oracle.com>; core-libs-dev at openjdk.java.net; i18n-dev at openjdk.java.net
Subject: Re: <i18n dev> RFR: 8154520: java.time: appendLocalizedOffset() should return the localized "GMT" string
Here are my comments to the webrev:
- No need to define Locale_US as a static field, nor have it in the test data (data_print_localized) then pass it as an argument to the test.
Specifying Locale.US in line 572, 578, and 586 should suffice.
- Copyright year is 2019.
- It would be nice if some comments that reads something like "This test relies on the localized text of "GMT" in the CLDR."
- Test class (and the description) should include "Localized", as it is testing the implementation of localized version of OffsetIdPrinterParser.
- Line 64-76: I prefer just instantiating them in the test data, not defining a static field for each, unless there will be duplicate in the test data.
- Line 111, 118, 124, 132: I believe the locale parameter is required.
Make sure that with Objects.requireNonNull(), or fail if it's null.
On 7/2/19 7:32 AM, Thejasvi Voniadka wrote:
> Hi Naoto,
> Thank you for the review. I have performed the modifications, and here is the updated webrev:
> I have moved the new tests from TCK area. I have also updated the current TCK test to explicitly pass Locale.US while calling format.
> -----Original Message-----
> From: Naoto Sato
> Sent: Monday, July 01, 2019 9:02 PM
> To: Thejasvi Voniadka <thejasvi.v.voniadka at oracle.com>;
> core-libs-dev at openjdk.java.net; i18n-dev at openjdk.java.net
> Subject: Re: <i18n dev> RFR: 8154520: java.time:
> appendLocalizedOffset() should return the localized "GMT" string
> Hi Thejasvi,
> Thanks for fixing this.
> Since those new test cases depend on the CLDR localization, which might change in other implementations, those test cases should be in jdk/java/time/test directory, as "tck" tests should only test the spec.
> Please create a new test case for this in the "test" directory (with @modules jdk.localedata directive) similar to the existing TCK one. Also the current test in the TCK should enforce that it runs in Locale.US so that the result should match "GMT."
> On 6/28/19 5:59 AM, Thejasvi Voniadka wrote:
>> Request you to please review this change.
>> JBS: https://bugs.openjdk.java.net/browse/JDK-8154520
>> Description: At present, the "DateTimeFormatterBuilder.appendLocalizedOffset()" method formulates the base string as "GMT", without accounting for locale-specific transformations. This change is to return the localized version of "GMT" instead. So for example, instead of returning "GMT +5.30", it may now return "XXXX +5.30" where "XXXX" is the localized string for "GMT" for the locale associated with the formatter. I have used DateTimeTextProvider.getLocalizedResource() method to return the "gmtZeroFormat" value from CLDR/LDML corresponding to the given locale. The code defaults to "GMT" in the absence of such a localized value.
>> Webrev: http://cr.openjdk.java.net/~vagarwal/8154520/webrev.1/
>> Additional notes: I preferred to update and reuse an existing test instead of creating a new one. It already has the niceties in place, and creating another method would mean some amount of code redundancy. However, if that's the recommended norm, then I can change it.
More information about the core-libs-dev