Hi Naoto
DateTimeFormatter.java, DateTimeFormatterBuilder.java, WeekFields.java.,
Calendar.java, Currency.java (and others)
——————
136 * the chronology and/or the zone are also overriden. If both "ca" and
"rg" are
137 * specified, the chronology from "ca" extension supersedes the implicit
one
138 * from "rg" extension.
————————
I would consider changing 137 “… from the “ca extension” and “… from the “rg”
extension"
I agree wth Rogers suggestion to use @{code} for the new javadoc if at all
possible
The tests look reasonable. I was curious why a few of the new tests such as
CalendarDataTest.java are not using TestNG where others are.
> On Nov 14, 2017, at 4:28 PM, Roger Riggs <[email protected]> wrote:
>
> Hi Naoto,
>
> A few comments, there is lots here to review.
>
> tools/cldrconverter/LDMLParseHandler.java:
> - 948: TODO?
>
>
> DateFormatSymbols.java:
> - 88: "overriden" -> "overridden"
>
> DecimalFormatSymbols:
> - 60: "rg" -> "nu"?
> - 62: "overriden" -> "overridden"
>
> NumberFormat.java:
> - 106: "a <code>NumberFormat</code>" -> "{@code NumberFormat}
>
> java/time/format/DateTimeFormatter.java:
> - 136, 561, 589, 1463: "overriden" -> "overridden"
>
> - 1486: long line, wrap please
>
> DateTimeFormatterBuilder.java:
> - 2168,2194: - 62: "overriden" -> "overridden"
>
> java/util/Calendar.java:
>
> 138 * They may also be specified explicitly through the methods for setting
> their
> 139 * values.
>
> java/util/spi/LocaleNameProvider.java:
> 167, 193: looks a bit odd to return null but maybe that's the default if not
> overridden
Yeah, could you explain when null would not be returned, or is this just
stubbed out code for now?
>
> It looks like a few misc changes are included too:
> - LauncherHelper: 271: getDisplayName()...
>
>
> CalendarDataProviderImpl.java:
> - 51, 58: Should these limit the value to 0-6 to avoid odd arithmetic;
> (Throw an exception if out of range)
>
> LocaleNameProviderImpl.java:
> - 176/186: Did you want to call super to do the RequiresNonNull checks? Or
> will the NPEs happen naturally.
>
> SPILocaleProviderAdapter.java:
> - 578, 585: assert seems like of useless here; if null, will NPE; only if
> asserts are enabled behavior will change to AssertError
>
> (I have not looked at the tests)
>
> Regards, Roger
>
>
>
> On 11/9/2017 6:34 PM, Naoto Sato wrote:
>> Kindly requesting reviews. I incorporated a fix to the following issue
>> raised by the test team:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8190918
>>
>> Here is the updated webrev:
>>
>> http://cr.openjdk.java.net/~naoto/8176841.8189134.8190918/webrev.04/
>>
>> And the webrev since the one below (to address 8190918):
>>
>> http://cr.openjdk.java.net/~naoto/8190918/
>>
>> Naoto
>>
>>
>>
>> On 11/2/17 2:42 PM, Naoto Sato wrote:
>>> Hello,
>>>
>>> Please review the proposed changes for the following issues:
>>>
>>> 8176841: Additional Unicode Language-Tag Extensions
>>> 8189134: New system properties for the default Locale extensions
>>>
>>> The proposed changeset is located at:
>>>
>>> http://cr.openjdk.java.net/~naoto/8176841/webrev.03/
>>>
>>> This serves as the implementation of JEP 314.
>>>
>>> Naoto
>>>
>>>
>>>
>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen|
Principal Member of Technical Staff | +1.781.442.2037
Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
[email protected] <mailto:[email protected]>
