[10] RFR 8176841: Additional Unicode Language-Tag Extensions

[Naoto Sato]

Thanks, Stephen.

On 11/14/17 3:22 PM, Stephen Colebourne wrote:
> I'd missed that this affects java.time.*
> 
> I believe that the changes to DateTimeFormatter are a mistake and will
> cause confusion. Currently, each withXxx() method is independent -
> they do not interact and operate on a single piece of state.
> 
> Currently, these two pieces of code have the same effect:
> 
>   formatter = formatter.withZone(zone).withLocale(locale);
>   formatter = formatter.withLocale(locale).withZone(zone);
> 
> With the webrev, that is no longer true.

OK.

> 
> While I understand the desire of the change, I believe it would be a
> serious mistake and a cause of end-user bugs.
> 
> The best I can suggest is adding a new method withLocalization(Locale)
> or applyLocalization(Locale) or localized(Locale), which is documented
> to replace multiple parts of the state in one go.

So even with the new suggested method,

formatter.withZone(locale).withLocalization(locale)
formatter.withLocalization(locale).withZone(locale)

would produce different formatters. Would it be OK, assuming along with 
some proper documentation?

> 
> 
> There are other oddities.
>   DateTimeFormatterBuilder.getLocalizedDateTimePattern() takes in a
> Chronology, thus the calendar system in the locale is ignored.
> Probably correct, but not called out in the docs.

Will document it.

> 
>   DateTimeFormatterBuilder.toFormatter(Locale, ResolverStyle,
> Chronology) has new logic to override the chronology. But this method
> is used indirectly from ofLocalizedTime() and friends which require
> the output to be ISO chronology. Thus the webrev would break the
> specification of those methods.

Would you suggest not interpreting extensions in this method? I am now 
inclined to just interpret locale extensions in the new suggested method 
for the java.time package.

> 
> TimeZoneNameUtility.convertLDMLShortID() is followed by ZoneId.of()
> which assumes that a full set of time-zones is loaded. But, the ZoneId
> initialization system allows the whole set of time-zones to be
> replaced, making this logic suspect, or at the very least needing
> extra consideration/testing.

Sure.

> 
> I think there is a case for
> CalendarDataUtility.retrieveFirstDayOfWeek() to return DayOfWeek, not
> int, but perhaps that is a separate change.

Agree.

Naoto

> 
> Stephen
> 
> 
> On 9 November 2017 at 23:34, Naoto Sato <naoto.sato at oracle.com> 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
>>>
>>>
>>>
>>