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

Tue Jul 9 14:09:35 UTC 2019

Hi Thejasvi,

Looks good, thanks for the updates

Roger

On 7/9/19 3:50 AM, Thejasvi Voniadka wrote:
> Hi Roger,
>
> Please find the updated webrev:
>
> http://cr.openjdk.java.net/~vagarwal/8154520/webrev.4/
>
>
> I have updated both lines 3924 and 3874 of .../java/time/format/DateTimeFormatterBuilder.java.
>
> I have reduced the number of tests to just 3, and the number of locales to just 1 in java/time/format/TestLocalizedOffsetPrinterParser.java. That should provide sufficient coverage towards the functionality.
>
>
>
> -----Original Message-----
> From: Thejasvi Voniadka
> Sent: Friday, July 05, 2019 9:16 AM
> To: Roger Riggs <roger.riggs 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 Roger,
>
>   
>
> Thank you for the review. I am in transit this weekend, but I will fix this and republish as soon as I can.
>
>   
>
>   
>
> From: Roger Riggs
> Sent: Wednesday, July 03, 2019 10:46 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,
>
> 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, HYPERLINK "mailto:naoto.sato at oracle.com"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 HYPERLINK "mailto:thejasvi.v.voniadka at oracle.com"<thejasvi.v.voniadka at oracle.com>; HYPERLINK "mailto:core-libs-dev at openjdk.java.net"core-libs-dev at openjdk.java.net; HYPERLINK "mailto:i18n-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 HYPERLINK "mailto:thejasvi.v.voniadka at oracle.com"<thejasvi.v.voniadka at oracle.com>;
> HYPERLINK "mailto:core-libs-dev at openjdk.java.net"core-libs-dev at openjdk.java.net; HYPERLINK "mailto:i18n-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.
>
>   