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

[Roger Riggs]

Hi,

Looks ok, but...

.../java/time/format/DateTimeFormatterBuilder.java
3924: needs a space in "if(" -> "if ("

java/time/format/TestLocalizedOffsetPrinterParser.java

I would cut the number of test cases to a minimum; only need to ensure 
the code is correct.
We don't need to be testing the CLDR data; it is just  pass through.
At least cut the number of different locale's used to cut the risk of 
unexpected maintenance.

Thanks, Roger



On 7/3/19 12:10 PM, naoto.sato at oracle.com wrote:
> 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.
>>>>