On Wed, 29 Apr 2026 19:31:06 GMT, Justin Lu <[email protected]> wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Addressing review comments
>
> src/java.base/share/classes/java/time/format/DateTimeFormatterPatternProvider.java
> line 1:
>
>> 1: /*
>
> Also, do we want this in an `spi` package similar to existing public SPI
> classes? Or do we not want to bother with that for a single class?
There already is `java.time.zone.ZoneRulesProvider` which resides in the same
package as the service user. So I followed the pattern here.
> src/java.base/share/classes/java/time/format/DateTimeFormatterPatternProvider.java
> line 47:
>
>> 45: */
>> 46:
>> 47: public abstract class DateTimeFormatterPatternProvider extends
>> LocaleServiceProvider {
>
> I presume it is an intentional decision to not add this class to the
> `spiClasses` array within `LocaleServiceProviderPool` because this is a more
> specific provider within `DateTimeFormatter`, the provider does not map 1-1
> with a corresponding public class, and `DateTimeFormatter` itself does not
> expose a `getAvailableLocales` method? (Even if an SPI that included a locale
> for this provider is now omitted from `Locale.getAvailableLocales()`.
No, it was an overlook. Added the class to `spiClasses` along with one other
missing one (`CalendarNameProvider`)
> src/java.base/share/classes/java/time/format/DateTimeFormatterPatternProvider.java
> line 74:
>
>> 72: * @throws NullPointerException if {@code calType} or {@code locale}
>> is
>> 73: * {@code null}.
>> 74: * @see Chronology#getCalendarType()
>
> I think this method should provide `@see` tags to a corresponding method in
> both `DateTimeFormatter` and `DateTimeFormatterBuilder`. The other method
> should as well (although it already does for (`DateTimeFormatterBuilder`).
Since this SPI is not common for most users. I'd rather not list in those APIs
> src/java.base/share/classes/java/time/format/DateTimeFormatterPatternProvider.java
> line 76:
>
>> 74: * @see Chronology#getCalendarType()
>> 75: */
>> 76: public abstract String getDateTimeFormatterPattern(FormatStyle
>> dateStyle,
>
> While not relevant to the JDK implementations (I believe), should we specify
> what happens when the given styles are not supported, such that SPI
> implementations have contract guidance to if they fall under that case?
> (Similar to how the pattern template method specifies the behavior when the
> pattern is non supported.)
Added `DateTimeException` clause
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/30979#discussion_r3164349078
PR Review Comment: https://git.openjdk.org/jdk/pull/30979#discussion_r3164353084
PR Review Comment: https://git.openjdk.org/jdk/pull/30979#discussion_r3164674348
PR Review Comment: https://git.openjdk.org/jdk/pull/30979#discussion_r3164654994
