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.vonia...@oracle.com>; core-libs-...@openjdk.java.net; i18n-dev@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.vonia...@oracle.com>; > core-libs-...@openjdk.java.net; i18n-dev@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. >>
