<i18n dev> RFR: 8154520: java.time: appendLocalizedOffset() should return the localized "GMT" string

[Thejasvi Voniadka]

Hi Naoto,

Thank you for the review. Please find the updated webrev:

http://cr.openjdk.java.net/~vagarwal/8154520/webrev.3/


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.



-----Original Message-----
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

Hi Thejasvi,

Here are my comments to the webrev:

TCKOffsetPrinterParser.java

- 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.

TestOffsetPrinterParser.java

- 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.

Naoto

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:
> 
> http://cr.openjdk.java.net/~vagarwal/8154520/webrev.2/
> 
> 
> 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."
> 
> Naoto
> 
> On 6/28/19 5:59 AM, Thejasvi Voniadka wrote:
>> Hi,
>>
>> 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.
>>