[15] RFR: 8246662: Test java/time/test/java/time/format/TestUnicodeExtension.java failed on japanese locale.

[naoto.sato at oracle.com]

Hi Joe,

On 6/9/20 11:54 AM, Joe Wang wrote:
> Looks fine to me for the implementation of the method.

Thank you for the review!

> 
> It's interesting to observe that the withXXXX methods, as demonstrated 
> in the test, always create a new DateTimeFormatter instance. The Javadoc 
> also said so, e.g. "Returns a copy of this formatter with a new XXXX", 
> and by "a copy" it meant a new instance. Now the localizedBy method also 
> "Returns a copy of this formatter", but it may actually returns this 
> formatter.  Would it confuse users (into thinking they always get a new 
> instance)?  Would we need to amend the javadoc a bit, e.g. indicating it 
> returns this formatter if all of the above aspects are the same? (or 
> otherwise a new instance is returned?)

Yes we would, but it's not specific to localizedBy() method but common 
to all methods. So I would rather follow the same pattern with other 
methods for this changeset.

> 
> The localizedBy method seems to me it's not "Unlike the withLocale 
> method" in terms of "producing a different formatter". The difference 
> lies in the attributes demonstrated in this changeset, e.g. zone, chrono 
> and decimalStyle will be from the specified locale in localizedBy, while 
> with the withLocale method, they are inherited. In other words, the 
> withXXXX methods only changes the specified field.

That's exactly why this localzedBy() was introduced. withLocale() only 
replaces the 'locale' field in the formatter, and no other fields/format 
patterns, even though they are locale related, are modified. "Unlike 
..." means to describe it.

Naoto

> 
> -Joe
> 
>>
>> Naoto
>>
>>>
>>> Thanks, Roger
>>>
>>>
>>>
>>> On 6/8/20 5:06 PM, naoto.sato at oracle.com wrote:
>>>> Hello,
>>>>
>>>> Please review the fix to the following issue:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8246662
>>>>
>>>> The proposed changeset is located at:
>>>>
>>>> https://cr.openjdk.java.net/~naoto/8246662/webrev.00/
>>>>
>>>> This is a regression caused by the fix to JDK-8244245, where the 
>>>> locale related fields in the formatter are now overridden by 
>>>> localizedBy() method. It was only comparing the locale object for 
>>>> the fast path, but it was not sufficient.
>>>>
>>>> Naoto
>>>
>