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