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

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

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

roger riggs
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

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
>>
>>
>>

Reply | Threaded
Open this post in threaded view
|

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

Lance Andersen
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 <[hidden email]> 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
[hidden email] <mailto:[hidden email]>



Reply | Threaded
Open this post in threaded view
|

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

Stephen Colebourne-2
In reply to this post by roger riggs
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.

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.


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.

 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.

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.

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

Stephen


On 9 November 2017 at 23:34, Naoto Sato <[hidden email]> 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
>>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Naoto Sato-2
In reply to this post by roger riggs
Thanks, Roger.

On 11/14/17 1:28 PM, Roger Riggs wrote:
> Hi Naoto,
>
> A few comments, there is lots here to review.
>
> tools/cldrconverter/LDMLParseHandler.java:
> - 948: TODO?

Will remove it.

> DateFormatSymbols.java:
>   - 88: "overriden" -> "overridden"
>
> DecimalFormatSymbols:
>   - 60: "rg" -> "nu"?

"nu" is mentioned in each constructor. "rg" is correct here.

>   - 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"

All of those typos will be corrected.

>
> java/util/Calendar.java:
>
> 138 * They may also be specified explicitly through the methods for
> setting their
> 139 * values.

What do you suggest here?

>
> java/util/spi/LocaleNameProvider.java:
>   167, 193: looks a bit odd to return null but maybe that's the default
> if not overridden

Yes, that's the default, and if it's null, then fall back to other
possibilities, either parent locale or other locale provider.

>
> It looks like a few misc changes are included too:
>   - LauncherHelper:  271: getDisplayName()...

I had discussed this with Kumar, and he agreed to fix this.

>
>
> 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

Will address those in the next webrev.

Naoto

>
> (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
>>>
>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|

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

Naoto Sato-2
In reply to this post by Lance Andersen
Thanks, Lance.

On 11/14/17 2:34 PM, Lance Andersen wrote:

> 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
Will change them.

>
> 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.

For the new tests in the new "bcp47" directory, I used TestNG, for other
tests in existing directories, I followed the test method in those
directories. Not from any definitive reason.

Naoto

>
>> On Nov 14, 2017, at 4:28 PM, Roger Riggs <[hidden email]
>> <mailto:[hidden email]>> 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
> [hidden email] <mailto:[hidden email]>
>
>
>
Reply | Threaded
Open this post in threaded view
|

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

Naoto Sato-2
In reply to this post by Stephen Colebourne-2
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 <[hidden email]> 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
>>>
>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|

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

Stephen Colebourne-2
On 14 November 2017 at 23:58, Naoto Sato <[hidden email]> wrote:
> 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?

Thats why I suggested perhaps a different method name is needed, not
withXxx() to highlight the larger impact. eg. localizedBy(Locale)

>>   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.

Fundamentally, the tags you are processing are a problem for the
design of java.time formatters. The existing API is structured around
a narrow meaning of Locale for text input/output within the formatting
API.

Changing the behaviour of DateTimeFormatterBuilder.toFormatter(...)
methods could have some odd effects for end-user code. Where they are
currently just expecting the locale to be set to control text
input/output, it would suddenly affect the calendar system and
time-zone, which could break code/compatibility in certain cases.

I think that its OK to use the unicode tags in places like
WeekFields.of() or Chronology.of(). But for formatting, the change in
meaning is too great. Adding a single method (name TBD) makes more
sense.

There is a case to add ZoneId.ofLocale(Locale) to match
Chronology.ofLocale(Locale). However, the expectation would be that it
figures out a suitable time-zone for the country/region as well as
considering the -u-tz- tag, and I don't think you've got that data
available at present (but it would make a good follow on change).

Stephen
Reply | Threaded
Open this post in threaded view
|

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

Naoto Sato-2
On 11/14/17 4:44 PM, Stephen Colebourne wrote:

> On 14 November 2017 at 23:58, Naoto Sato <[hidden email]> wrote:
>> 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?
>
> Thats why I suggested perhaps a different method name is needed, not
> withXxx() to highlight the larger impact. eg. localizedBy(Locale)

OK. Will come up with a draft. Essentially what the new method does is
what withLocale() does, plus replacing fields specified with Unicode
extensions (calendar/timezone/region override)

>
>>>    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.
>
> Fundamentally, the tags you are processing are a problem for the
> design of java.time formatters. The existing API is structured around
> a narrow meaning of Locale for text input/output within the formatting
> API.
>
> Changing the behaviour of DateTimeFormatterBuilder.toFormatter(...)
> methods could have some odd effects for end-user code. Where they are
> currently just expecting the locale to be set to control text
> input/output, it would suddenly affect the calendar system and
> time-zone, which could break code/compatibility in certain cases.

I've decided to retract changes in
DateTimeFormatter/DateTimeFormatterBuilder currently in the webrev, with
adding some text noting Unicode extensions are ignored in those methods.

>
> I think that its OK to use the unicode tags in places like
> WeekFields.of() or Chronology.of(). But for formatting, the change in
> meaning is too great. Adding a single method (name TBD) makes more
> sense
Will keep the changes in WeekFields.of()/Chronology.of()

>
> There is a case to add ZoneId.ofLocale(Locale) to match
> Chronology.ofLocale(Locale). However, the expectation would be that it
> figures out a suitable time-zone for the country/region as well as
> considering the -u-tz- tag, and I don't think you've got that data
> available at present (but it would make a good follow on change).

Yes, we don't yet have that mechanism to derive time zone based on the
region. I think that's out of this JEP's scope.

Naoto
Reply | Threaded
Open this post in threaded view
|

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

Naoto Sato-2
Here is the proposed spec changes:

http://cr.openjdk.java.net/~naoto/8191349/specdiff.00/overview-summary.html

Naoto


On 11/15/17 10:06 AM, Naoto Sato wrote:

> On 11/14/17 4:44 PM, Stephen Colebourne wrote:
>> On 14 November 2017 at 23:58, Naoto Sato <[hidden email]> wrote:
>>> 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?
>>
>> Thats why I suggested perhaps a different method name is needed, not
>> withXxx() to highlight the larger impact. eg. localizedBy(Locale)
>
> OK. Will come up with a draft. Essentially what the new method does is
> what withLocale() does, plus replacing fields specified with Unicode
> extensions (calendar/timezone/region override)
>
>>
>>>>    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.
>>
>> Fundamentally, the tags you are processing are a problem for the
>> design of java.time formatters. The existing API is structured around
>> a narrow meaning of Locale for text input/output within the formatting
>> API.
>>
>> Changing the behaviour of DateTimeFormatterBuilder.toFormatter(...)
>> methods could have some odd effects for end-user code. Where they are
>> currently just expecting the locale to be set to control text
>> input/output, it would suddenly affect the calendar system and
>> time-zone, which could break code/compatibility in certain cases.
>
> I've decided to retract changes in
> DateTimeFormatter/DateTimeFormatterBuilder currently in the webrev, with
> adding some text noting Unicode extensions are ignored in those methods.
>
>>
>> I think that its OK to use the unicode tags in places like
>> WeekFields.of() or Chronology.of(). But for formatting, the change in
>> meaning is too great. Adding a single method (name TBD) makes more
>> sense
> Will keep the changes in WeekFields.of()/Chronology.of()
>
>>
>> There is a case to add ZoneId.ofLocale(Locale) to match
>> Chronology.ofLocale(Locale). However, the expectation would be that it
>> figures out a suitable time-zone for the country/region as well as
>> considering the -u-tz- tag, and I don't think you've got that data
>> available at present (but it would make a good follow on change).
>
> Yes, we don't yet have that mechanism to derive time zone based on the
> region. I think that's out of this JEP's scope.
>
> Naoto
Reply | Threaded
Open this post in threaded view
|

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

roger riggs
Hi Naoto,

- The word "designated" is unnecessary and inconsistent with the rest of
the java.time API doc.
    "designated locale" -> "locale"
     I would have written: "Unicode extensions in the locale are ignored."

- localizedBy(Locale):
   The first sentence should be more specific.
    " Returns a copy of this formatter with localized values of the
locale, calendar, region and/or timezone, that supercede values in this
formatter."
    to make it clearer that is is not a simple Locale substitution.
   And update the @return to say the same

   I would omit the example, to avoid the reader puzzling over what it
is that is changed.  Or update the comment
   to reinforce the point that the values are replaced when the
formatter is copied.

$.02, Roger


On 11/15/2017 4:18 PM, Naoto Sato wrote:

> Here is the proposed spec changes:
>
> http://cr.openjdk.java.net/~naoto/8191349/specdiff.00/overview-summary.html 
>
>
> Naoto
>
>
> On 11/15/17 10:06 AM, Naoto Sato wrote:
>> On 11/14/17 4:44 PM, Stephen Colebourne wrote:
>>> On 14 November 2017 at 23:58, Naoto Sato <[hidden email]> wrote:
>>>> 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?
>>>
>>> Thats why I suggested perhaps a different method name is needed, not
>>> withXxx() to highlight the larger impact. eg. localizedBy(Locale)
>>
>> OK. Will come up with a draft. Essentially what the new method does
>> is what withLocale() does, plus replacing fields specified with
>> Unicode extensions (calendar/timezone/region override)
>>
>>>
>>>>> 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.
>>>
>>> Fundamentally, the tags you are processing are a problem for the
>>> design of java.time formatters. The existing API is structured around
>>> a narrow meaning of Locale for text input/output within the formatting
>>> API.
>>>
>>> Changing the behaviour of DateTimeFormatterBuilder.toFormatter(...)
>>> methods could have some odd effects for end-user code. Where they are
>>> currently just expecting the locale to be set to control text
>>> input/output, it would suddenly affect the calendar system and
>>> time-zone, which could break code/compatibility in certain cases.
>>
>> I've decided to retract changes in
>> DateTimeFormatter/DateTimeFormatterBuilder currently in the webrev,
>> with adding some text noting Unicode extensions are ignored in those
>> methods.
>>
>>>
>>> I think that its OK to use the unicode tags in places like
>>> WeekFields.of() or Chronology.of(). But for formatting, the change in
>>> meaning is too great. Adding a single method (name TBD) makes more
>>> sense
>> Will keep the changes in WeekFields.of()/Chronology.of()
>>
>>>
>>> There is a case to add ZoneId.ofLocale(Locale) to match
>>> Chronology.ofLocale(Locale). However, the expectation would be that it
>>> figures out a suitable time-zone for the country/region as well as
>>> considering the -u-tz- tag, and I don't think you've got that data
>>> available at present (but it would make a good follow on change).
>>
>> Yes, we don't yet have that mechanism to derive time zone based on
>> the region. I think that's out of this JEP's scope.
>>
>> Naoto

Reply | Threaded
Open this post in threaded view
|

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

Naoto Sato-2
Thanks, Roger. Updated the specdiff as suggested:

http://cr.openjdk.java.net/~naoto/8191349/specdiff.01/overview-summary.html

Naoto

On 11/15/17 2:06 PM, Roger Riggs wrote:

> Hi Naoto,
>
> - The word "designated" is unnecessary and inconsistent with the rest of
> the java.time API doc.
>     "designated locale" -> "locale"
>      I would have written: "Unicode extensions in the locale are ignored."
>
> - localizedBy(Locale):
>    The first sentence should be more specific.
>     " Returns a copy of this formatter with localized values of the
> locale, calendar, region and/or timezone, that supercede values in this
> formatter."
>     to make it clearer that is is not a simple Locale substitution.
>    And update the @return to say the same
>
>    I would omit the example, to avoid the reader puzzling over what it
> is that is changed.  Or update the comment
>    to reinforce the point that the values are replaced when the
> formatter is copied.
>
> $.02, Roger
>
>
> On 11/15/2017 4:18 PM, Naoto Sato wrote:
>> Here is the proposed spec changes:
>>
>> http://cr.openjdk.java.net/~naoto/8191349/specdiff.00/overview-summary.html 
>>
>>
>> Naoto
>>
>>
>> On 11/15/17 10:06 AM, Naoto Sato wrote:
>>> On 11/14/17 4:44 PM, Stephen Colebourne wrote:
>>>> On 14 November 2017 at 23:58, Naoto Sato <[hidden email]> wrote:
>>>>> 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?
>>>>
>>>> Thats why I suggested perhaps a different method name is needed, not
>>>> withXxx() to highlight the larger impact. eg. localizedBy(Locale)
>>>
>>> OK. Will come up with a draft. Essentially what the new method does
>>> is what withLocale() does, plus replacing fields specified with
>>> Unicode extensions (calendar/timezone/region override)
>>>
>>>>
>>>>>> 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.
>>>>
>>>> Fundamentally, the tags you are processing are a problem for the
>>>> design of java.time formatters. The existing API is structured around
>>>> a narrow meaning of Locale for text input/output within the formatting
>>>> API.
>>>>
>>>> Changing the behaviour of DateTimeFormatterBuilder.toFormatter(...)
>>>> methods could have some odd effects for end-user code. Where they are
>>>> currently just expecting the locale to be set to control text
>>>> input/output, it would suddenly affect the calendar system and
>>>> time-zone, which could break code/compatibility in certain cases.
>>>
>>> I've decided to retract changes in
>>> DateTimeFormatter/DateTimeFormatterBuilder currently in the webrev,
>>> with adding some text noting Unicode extensions are ignored in those
>>> methods.
>>>
>>>>
>>>> I think that its OK to use the unicode tags in places like
>>>> WeekFields.of() or Chronology.of(). But for formatting, the change in
>>>> meaning is too great. Adding a single method (name TBD) makes more
>>>> sense
>>> Will keep the changes in WeekFields.of()/Chronology.of()
>>>
>>>>
>>>> There is a case to add ZoneId.ofLocale(Locale) to match
>>>> Chronology.ofLocale(Locale). However, the expectation would be that it
>>>> figures out a suitable time-zone for the country/region as well as
>>>> considering the -u-tz- tag, and I don't think you've got that data
>>>> available at present (but it would make a good follow on change).
>>>
>>> Yes, we don't yet have that mechanism to derive time zone based on
>>> the region. I think that's out of this JEP's scope.
>>>
>>> Naoto
>
Reply | Threaded
Open this post in threaded view
|

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

Lance Andersen
Hi Naoto

localizedBy, i would suggest changing:

-   “If the new locale contains “ca”…”  to “if the new locale contains the “ca”…”
- “Unlike withLocale method” to Unlike the withLocale method”

Best
Lance
> On Nov 15, 2017, at 5:36 PM, Naoto Sato <[hidden email]> wrote:
>
> http://cr.openjdk.java.net/~naoto/8191349/specdiff.01/overview-summary.html <http://cr.openjdk.java.net/~naoto/8191349/specdiff.01/overview-summary.html>
 <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
[hidden email] <mailto:[hidden email]>



Reply | Threaded
Open this post in threaded view
|

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

Naoto Sato-2
Thanks, Lance. Corrected as suggested.

http://cr.openjdk.java.net/~naoto/8191349/specdiff.02/overview-summary.html

Also, I inserted "@since 10."

Naoto

On 11/15/17 3:06 PM, Lance Andersen wrote:

> Hi Naoto
>
> localizedBy, i would suggest changing:
>
> -   “If the new locale contains “ca”…”  to “if the new locale contains
> the “ca”…”
> - “Unlike withLocale method” to Unlike the withLocale method”
>
> Best
> Lance
>> On Nov 15, 2017, at 5:36 PM, Naoto Sato <[hidden email]
>> <mailto:[hidden email]>> wrote:
>>
>> http://cr.openjdk.java.net/~naoto/8191349/specdiff.01/overview-summary.html
>
> <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
> [hidden email] <mailto:[hidden email]>
>
>
>
Reply | Threaded
Open this post in threaded view
|

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

Stephen Colebourne-2
I've had a longer think about how to integrate this. Its very tricky,
as the unicode extensions create complex conflicts.

In general, my view is that Locale is too complex and overloaded with
different levels of meaning. Perhaps a different class should have
been added for more complex localization settings. It seems like an
are rife for puzzlers/security issues as unicode extensions are going
to be hugely undertested. (Most developers think of a Locale as
language+territory, and will only test for that, if they test at all).
It will be all too easy for text to appear on web pages or reports in
an unexpected calendar system or time-zone as a result of this change.
It also means that numbers might be output in an unexpected numbering
system or currency.

The tension is between what the developer has thought of and tested
for, and what a user might pass in. The mechanism for passing in these
extensions is partly subtle, being via the default locale. Although
even via an explicit locale, I suspect very few developers will be
sanitizing a Locale input from a user.

AFAICT, unicode extensions are not widely used at present. I can only
find use of "ca" in Chronology.ofLocale() and Calendar.getInstance().
So this is the key moment for deciding how they are used.

We are discussing 6 unicode extensions: "ca" (calendar system), "tz"
(time-zone), "cu" (currency), "nu" (numbering system), "fw" (first day
of week) and "rg" (region override).

Chronology, ZoneId, TimeZone and Currency ("ca", "tz", "cu") are not
really aspects of formatting at all, they are explicit aspects of the
associated value. eg. a ZonedDateTime explicitly has a chronology of
IsoChronology and a specific time zone. A Money class (Java will have
one at some point, probably after value types are added) explicitly
has a Currency.

You *really, really, really* don't want a unicode extension locale to
be changing the format of a Money object - turning $300 to £300 just
because the user had -u-cu-gbp in their locale. The same applies to
the formatting of dates and times - the chronology and time-zone are
part of the value being formatted, not the format.

This just leaves "nu" (numbering system) where there is a case for the
unicode extension to be picked up directly. But doing so would cause a
LocalDate to be output as ????-??-?? (where ? is the digit for another
numbering system), While I think that should be supported, should it
be picked up automatically via the locale? No, it should be explicitly
selected by using DateTimeFormatter.withDecimalStyle() - which the
webrev does do.

The distinction here is that DateTimeFormatter (and our theoretical
MoneyFormatter) are formatting the whole state of the value object,
whereas DateFormatter and NumberFormatter essentially just format a
single number, using extra information alongside to help.

So, what to do? Some of these are new changes, some just need testing:

1) The changes to `Calendar.getInstance()` are not documented wrt
"tz". Currently it says it results in the "default time zone", which
isn't true anymore. I think this may be a behavioural compatibility
issue.

2) `DecimalStyle.ofLocale(Locale)` should use "nu" but does not.

3) `DateTimeFormatter.localizedBy(Locale)` should use "ca" to call
`withChronology`, `tz` to call `withZoneId` and `nu` to call
`withDecimalStyle`. This is a change to the CSR.

4) The phrase "Unicode extensions in the locale are ignored." would
apply to lots of methods across the JDK. In particular for this
webrev, it would need to be added to a lot more methods, such as the
default locale methods. Perhaps the phrase could be "If the locale has
unicode extensions, they are not used", which is slightly easier to
read. Or perhaps it shouldn't be added at all in most cases (ie.
unless unicode extensions are mentioned, assume that they are not
used.)

5) The withLocale(Locale) change should say "The locale is stored as
passed in, without further processing. If the locale has unicode
extensions, they may be used later in text processing. To set the
chronology, time-zone and decimal style from unicode extensions, see
localizedBy()."

6) The "rg" extension (and no other extensions) should be used when
looking up data to output these:
- DateTimeFormatterBuilder.appendText()
- DateTimeFormatterBuilder.appendLocalizedOffset()
- DateTimeFormatterBuilder.appendZoneText()
- DateTimeFormatterBuilder.appendChronologyText()
- DateTimeFormatterBuilder.appendLocalized()
- DateTimeFormatterBuilder.getLocalizedDateTimePattern()
This may be the case, but tests should ensure it.

7) WeekBasedFieldPrinterParser should use "fw"/"rg", which it already
does via WeekFields.of(Locale)

8) For clarity of future code, two additional methods would be useful:
- DateTimeFormatterBuilder.toFormatterIsoRoot()
- DateTimeFormatterBuilder.toFormatterIso(Locale)
These would set the chronology of the resulting DTF to be
IsoChronology.INSTANCE. The "root" variant would use `Locale.ROOT`.

9) Consider more generally what happens if TimeZone.getDefault() does
not match the "tz" extension of Locale.getDefault().

10) Consider how localizedBy(Locale) operates. Is it the same as
formatter
  .withChronology(Chronology.ofLocale(loc))
  .withZoneId(ZoneId.ofLocale(loc))
  .withDecimalStyle(DecimalStyle.of(loc))
or does it only set the chronology if "ca" if found, and only set
time-zone if "tz" is found and only set decimal style if "nu" is
found.

Perhaps this webrev should be broken into two or more parts, as it is
very large?


In summary, going forward it should be recognised that there is a
separation/difference between formatters of full values
(Money/Temporal) and old-style formatters of single numbers
(DateFormat, NumberFormat). Some extensions, "ca", "tz", "cu", are
part of the value in Money/Temporal, and so can't be overridden by the
locale. Methods like Chronology.ofLocale or ZoneId.of(Locale) are good
places to use the extensions, as they are explicit about what happens.

Stephen


On 15 November 2017 at 23:31, Naoto Sato <[hidden email]> wrote:

> Thanks, Lance. Corrected as suggested.
>
> http://cr.openjdk.java.net/~naoto/8191349/specdiff.02/overview-summary.html
> Also, I inserted "@since 10."
>
> Naoto
>
> On 11/15/17 3:06 PM, Lance Andersen wrote:
>>
>> Hi Naoto
>>
>> localizedBy, i would suggest changing:
>>
>> -   “If the new locale contains “ca”…”  to “if the new locale contains the
>> “ca”…”
>> - “Unlike withLocale method” to Unlike the withLocale method”
>>
>> Best
>> Lance
>>>
>>> On Nov 15, 2017, at 5:36 PM, Naoto Sato <[hidden email]
>>> <mailto:[hidden email]>> wrote:
>>>
>>>
>>> http://cr.openjdk.java.net/~naoto/8191349/specdiff.01/overview-summary.html
>>
>>
>> <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
>> [hidden email] <mailto:[hidden email]>
>>
>>
>>
>
Reply | Threaded
Open this post in threaded view
|

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

Naoto Sato-2
Hi Stephen, please see my comments below.

On 11/20/17 5:27 AM, Stephen Colebourne wrote:

> I've had a longer think about how to integrate this. Its very tricky,
> as the unicode extensions create complex conflicts.
>
> In general, my view is that Locale is too complex and overloaded with
> different levels of meaning. Perhaps a different class should have
> been added for more complex localization settings. It seems like an
> are rife for puzzlers/security issues as unicode extensions are going
> to be hugely undertested. (Most developers think of a Locale as
> language+territory, and will only test for that, if they test at all).
> It will be all too easy for text to appear on web pages or reports in
> an unexpected calendar system or time-zone as a result of this change.
> It also means that numbers might be output in an unexpected numbering
> system or currency.
>
> The tension is between what the developer has thought of and tested
> for, and what a user might pass in. The mechanism for passing in these
> extensions is partly subtle, being via the default locale. Although
> even via an explicit locale, I suspect very few developers will be
> sanitizing a Locale input from a user.
>
> AFAICT, unicode extensions are not widely used at present. I can only
> find use of "ca" in Chronology.ofLocale() and Calendar.getInstance().
> So this is the key moment for deciding how they are used.
>
> We are discussing 6 unicode extensions: "ca" (calendar system), "tz"
> (time-zone), "cu" (currency), "nu" (numbering system), "fw" (first day
> of week) and "rg" (region override).
>
> Chronology, ZoneId, TimeZone and Currency ("ca", "tz", "cu") are not
> really aspects of formatting at all, they are explicit aspects of the
> associated value. eg. a ZonedDateTime explicitly has a chronology of
> IsoChronology and a specific time zone. A Money class (Java will have
> one at some point, probably after value types are added) explicitly
> has a Currency.
>
> You *really, really, really* don't want a unicode extension locale to
> be changing the format of a Money object - turning $300 to £300 just
> because the user had -u-cu-gbp in their locale. The same applies to
> the formatting of dates and times - the chronology and time-zone are
> part of the value being formatted, not the format.
>
> This just leaves "nu" (numbering system) where there is a case for the
> unicode extension to be picked up directly. But doing so would cause a
> LocalDate to be output as ????-??-?? (where ? is the digit for another
> numbering system), While I think that should be supported, should it
> be picked up automatically via the locale? No, it should be explicitly
> selected by using DateTimeFormatter.withDecimalStyle() - which the
> webrev does do.
>
> The distinction here is that DateTimeFormatter (and our theoretical
> MoneyFormatter) are formatting the whole state of the value object,
> whereas DateFormatter and NumberFormatter essentially just format a
> single number, using extra information alongside to help.
>
> So, what to do? Some of these are new changes, some just need testing:
>
> 1) The changes to `Calendar.getInstance()` are not documented wrt
> "tz". Currently it says it results in the "default time zone", which
> isn't true anymore. I think this may be a behavioural compatibility
> issue.

Explicitly document that it will use the time zone in locale.

>
> 2) `DecimalStyle.ofLocale(Locale)` should use "nu" but does not.

Document it in the javadoc.

>
> 3) `DateTimeFormatter.localizedBy(Locale)` should use "ca" to call
> `withChronology`, `tz` to call `withZoneId` and `nu` to call
> `withDecimalStyle`. This is a change to the CSR.

Besides that "nu" needs to be spec'ed out, isn't calling withXXXX() an
implementation note?

>
> 4) The phrase "Unicode extensions in the locale are ignored." would
> apply to lots of methods across the JDK. In particular for this
> webrev, it would need to be added to a lot more methods, such as the
> default locale methods. Perhaps the phrase could be "If the locale has
> unicode extensions, they are not used", which is slightly easier to
> read. Or perhaps it shouldn't be added at all in most cases (ie.
> unless unicode extensions are mentioned, assume that they are not
> used.)

Removed the phrase.

>
> 5) The withLocale(Locale) change should say "The locale is stored as
> passed in, without further processing. If the locale has unicode
> extensions, they may be used later in text processing. To set the
> chronology, time-zone and decimal style from unicode extensions, see
> localizedBy()."

Added.

>
> 6) The "rg" extension (and no other extensions) should be used when
> looking up data to output these:
> - DateTimeFormatterBuilder.appendText()
> - DateTimeFormatterBuilder.appendLocalizedOffset()
> - DateTimeFormatterBuilder.appendZoneText()
> - DateTimeFormatterBuilder.appendChronologyText()
> - DateTimeFormatterBuilder.appendLocalized()
> - DateTimeFormatterBuilder.getLocalizedDateTimePattern()
> This may be the case, but tests should ensure it.

Will need to add tests.

>
> 7) WeekBasedFieldPrinterParser should use "fw"/"rg", which it already
> does via WeekFields.of(Locale)

Not sure what this means. Where is the file located?

>
> 8) For clarity of future code, two additional methods would be useful:
> - DateTimeFormatterBuilder.toFormatterIsoRoot()
> - DateTimeFormatterBuilder.toFormatterIso(Locale)
> These would set the chronology of the resulting DTF to be
> IsoChronology.INSTANCE. The "root" variant would use `Locale.ROOT`.

This would be a nice addition.

>
> 9) Consider more generally what happens if TimeZone.getDefault() does
> not match the "tz" extension of Locale.getDefault().

This should be considered case by case, and document on each occasion
which would be honored.

>
> 10) Consider how localizedBy(Locale) operates. Is it the same as
> formatter
>    .withChronology(Chronology.ofLocale(loc))
>    .withZoneId(ZoneId.ofLocale(loc))
>    .withDecimalStyle(DecimalStyle.of(loc))
> or does it only set the chronology if "ca" if found, and only set
> time-zone if "tz" is found and only set decimal style if "nu" is
> found.

IIRC, the localizedBy() is added so that withLocale() would behave as it
is now. I think localizedBy() should also have the same effect as
withLocale if the specified locale do not contain any
calendar/timezone/numberingSystem extensions. Otherwise, say
localizedBy(Locale.JAPAN) would be no-operation.

As we discussed in the previous email, we don't have ZoneId.ofLocale() yet.

>
> Perhaps this webrev should be broken into two or more parts, as it is
> very large?
>
>
> In summary, going forward it should be recognised that there is a
> separation/difference between formatters of full values
> (Money/Temporal) and old-style formatters of single numbers
> (DateFormat, NumberFormat). Some extensions, "ca", "tz", "cu", are
> part of the value in Money/Temporal, and so can't be overridden by the
> locale. Methods like Chronology.ofLocale or ZoneId.of(Locale) are good
> places to use the extensions, as they are explicit about what happens.

Here is the spec diff from the previous one. Please let me know your
thought.

http://cr.openjdk.java.net/~naoto/8191349/specdiff.03/overview-summary.html

Naoto

>
> Stephen
>
>
> On 15 November 2017 at 23:31, Naoto Sato <[hidden email]> wrote:
>> Thanks, Lance. Corrected as suggested.
>>
>> http://cr.openjdk.java.net/~naoto/8191349/specdiff.02/overview-summary.html
>> Also, I inserted "@since 10."
>>
>> Naoto
>>
>> On 11/15/17 3:06 PM, Lance Andersen wrote:
>>>
>>> Hi Naoto
>>>
>>> localizedBy, i would suggest changing:
>>>
>>> -   “If the new locale contains “ca”…”  to “if the new locale contains the
>>> “ca”…”
>>> - “Unlike withLocale method” to Unlike the withLocale method”
>>>
>>> Best
>>> Lance
>>>>
>>>> On Nov 15, 2017, at 5:36 PM, Naoto Sato <[hidden email]
>>>> <mailto:[hidden email]>> wrote:
>>>>
>>>>
>>>> http://cr.openjdk.java.net/~naoto/8191349/specdiff.01/overview-summary.html
>>>
>>>
>>> <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
>>> [hidden email] <mailto:[hidden email]>
>>>
>>>
>>>
>>
Reply | Threaded
Open this post in threaded view
|

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

Stephen Colebourne-2
On 21 November 2017 at 01:45, Naoto Sato <[hidden email]> wrote:
>> 2) `DecimalStyle.ofLocale(Locale)` should use "nu" but does not.
>
> Document it in the javadoc.

Javadoc looks good, but the webrev didn't contain matching code last
time I looked.

>> 3) `DateTimeFormatter.localizedBy(Locale)` should use "ca" to call
>> `withChronology`, `tz` to call `withZoneId` and `nu` to call
>> `withDecimalStyle`. This is a change to the CSR.
>
> Besides that "nu" needs to be spec'ed out, isn't calling withXXXX() an
> implementation note?

The revised text ends with "Same is true for the "nu" extension.", but
doesn't mention the case where both "tz" and "rg" are present.

>> 7) WeekBasedFieldPrinterParser should use "fw"/"rg", which it already
>> does via WeekFields.of(Locale)
>
> Not sure what this means. Where is the file located?

WeekBasedFieldPrinterParser is an inner class of DateTimeFormatterBuilder

>> 10) Consider how localizedBy(Locale) operates.
>
> IIRC, the localizedBy() is added so that withLocale() would behave as it is
> now. I think localizedBy() should also have the same effect as withLocale if
> the specified locale do not contain any calendar/timezone/numberingSystem
> extensions. Otherwise, say localizedBy(Locale.JAPAN) would be no-operation.

OK, I agree. localizedBy(locale) is the same as withLocale(locale)
unless there are "ca", "tz" or "nu", in which case the matching
element is updated.

thanks
Stephen
Reply | Threaded
Open this post in threaded view
|

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

Naoto Sato-2
Thanks, Stephen.

On 11/21/17 1:35 AM, Stephen Colebourne wrote:
> On 21 November 2017 at 01:45, Naoto Sato <[hidden email]> wrote:
>>> 2) `DecimalStyle.ofLocale(Locale)` should use "nu" but does not.
>>
>> Document it in the javadoc.
>
> Javadoc looks good, but the webrev didn't contain matching code last
> time I looked.

I haven't updated the webrev yet, since it will involve new test cases.
I wanted to make sure the direction of the change was correct. Will
update the webrev soon.

>
>>> 3) `DateTimeFormatter.localizedBy(Locale)` should use "ca" to call
>>> `withChronology`, `tz` to call `withZoneId` and `nu` to call
>>> `withDecimalStyle`. This is a change to the CSR.
>>
>> Besides that "nu" needs to be spec'ed out, isn't calling withXXXX() an
>> implementation note?
>
> The revised text ends with "Same is true for the "nu" extension.", but
> doesn't mention the case where both "tz" and "rg" are present.

Since there is no ZoneId.ofLocale(rg), a region does not designate a
time zone (yet). So there would be no conflict between "tz" and "rg"
extensions.

>
>>> 7) WeekBasedFieldPrinterParser should use "fw"/"rg", which it already
>>> does via WeekFields.of(Locale)
>>
>> Not sure what this means. Where is the file located?
>
> WeekBasedFieldPrinterParser is an inner class of DateTimeFormatterBuilder

OK, thanks.

>
>>> 10) Consider how localizedBy(Locale) operates.
>>
>> IIRC, the localizedBy() is added so that withLocale() would behave as it is
>> now. I think localizedBy() should also have the same effect as withLocale if
>> the specified locale do not contain any calendar/timezone/numberingSystem
>> extensions. Otherwise, say localizedBy(Locale.JAPAN) would be no-operation.
>
> OK, I agree. localizedBy(locale) is the same as withLocale(locale)
> unless there are "ca", "tz" or "nu", in which case the matching
> element is updated.

Good.

Naoto