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

[naoto.sato at oracle.com]

Looks good.

Naoto

On 7/3/19 5:32 AM, Thejasvi Voniadka wrote:
> 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.
>>>