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.ri...@oracle.com>; core-libs-dev@openjdk.java.net; i18n-...@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.vonia...@oracle.com>; core-libs-dev@openjdk.java.net; i18n-...@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.s...@oracle.com"naoto.s...@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.vonia...@oracle.com";<thejasvi.v.vonia...@oracle.com>; HYPERLINK "mailto:core-libs-dev@openjdk.java.net"core-libs-dev@openjdk.java.net; HYPERLINK "mailto:i18n-...@openjdk.java.net"i18n-...@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.vonia...@oracle.com";<thejasvi.v.vonia...@oracle.com>; HYPERLINK "mailto:core-libs-dev@openjdk.java.net"core-libs-dev@openjdk.java.net; HYPERLINK "mailto:i18n-...@openjdk.java.net"i18n-...@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.
