On 7/13/2020 9:04 PM, [email protected] wrote:
Hi Joe,
On 7/13/20 7:28 PM, Joe Wang wrote:
On 7/13/2020 7:01 PM, [email protected] wrote:
Hi Joe,
Thank you for your review.
On 7/13/20 3:55 PM, Joe Wang wrote:
Hi Naoto,
Would it make sense to provide an additional test using the public
APIs similar to the one provided in the bug report? I'm sure yours
is correct and covers more cases than the original, but it would be
nice to have an actual use case and use the public APIs. The report
showed it was failed somewhere down the stream than when it is run
against the current build, which produces IAE "Too many pattern
letters: a" instead of what's reported.
I am not quite sure I got your suggestion, but the test case uses
the same API as in the bug report (LocaleProviders.java, line
421-428) that are supposed to catch two cases, i.e.,
ofLocalizedDate()/ofLocalizedTime() tests catch "unsupported
temporal field" exception, such as HourOfDay, and
ofLocalizedDateTime() catches "Too many pattern letters: aa" which
you saw in (possibly) en_US locale. To make it clearer, I added some
comments to it.
Yes, the test covered the cases. What I was suggesting was an
additional test that uses only public APIs similar to that in the bug
report, that is what users would do and how they may encounter this
issue, e.g.
System.setProperty("java.locale.providers", "HOST");
DateTimeFormatter formatter =
DateTimeFormatter.ofLocalizedDate(FormatStyle.FULL);
It's unlikely for an user application to refer to a private
package/class like sun.util.locale.provider.LocaleProviderAdapter.
I understand it's been that way for these test. But to me, a real
world use case is always nice to have. Your call whether you want to
add it or not. There's no missing coverage.
Now I got what you are talking about. The way the bug report is doing
is in fact discouraged, as once it is read at the startup, it won't be
read again with later setProperty() call. Apps are supposed to specify
the property as -D launcher parameter. (And the test case is doing it,
at line 184 of LocaleProvidersRun.java) Internal interface is used
only to tell whether HOST provider is actually used or not, so apps
should not depend on it.
I see. Thanks for the clarification. Also, nice comments to the test,
helpful for understanding the details if ever it's read again.
Best,
Joe
HostLocaleProviderAdapter_md:849 - 865: may be compacted into one
if statement, (bCal && getCalendarInfoWrapper(...) ||
getLocaleInfoWrapper(...)).
Quite right. Modified.
The updated webrev is located at:
https://cr.openjdk.java.net/~naoto/8248695/webrev.01/
Looks good to me.
Thanks!
Naoto
Best,
Joe
Naoto
Regards,
Joe
On 7/13/2020 5:54 AM, [email protected] wrote:
Ping.
On 7/7/20 3:55 PM, [email protected] wrote:
Hello,
Please review the fix to the following issue:
https://bugs.openjdk.java.net/browse/JDK-8248695
The proposed changeset is located at:
http://cr.openjdk.java.net/~naoto/8248695/webrev.00/
There were two causes that resulted in throwing exceptions. One
was that the Host adapter for Windows always produced Date and
Time combined patterns, so formatting a LocalDate ended up with
unsupported temporal field for HourOfDay (reported in the bug),
and the other cause was the pattern for am/pm was "aa", which was
not valid as a DateTimeFormatter pattern.
Besides these issues, localized DayOfWeek/AM_PM names have not
been correctly implemented in the host adapter. Now those names
are correctly returned from Windows.
Naoto
