RFR: 8176706: Additional Date-Time Formats [v3]

[Roger Riggs]

On Tue, 8 Feb 2022 19:08:45 GMT, Naoto Sato <naoto at openjdk.org> wrote:

>> Following the prior discussion [1], here is the PR for the subject enhancement. CSR has also been updated according to the suggestion.
>> 
>> [1] https://mail.openjdk.java.net/pipermail/core-libs-dev/2022-January/085175.html
>
> Naoto Sato has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fixed LocalizedPrinterParser.toString() to reflect requestedTemplate

src/java.base/share/classes/java/time/format/DateTimeFormatter.java line 742:

> 740:      * All pattern symbols are optional, and each pattern symbol represents the field it is in,
> 741:      * e.g., 'M' represents the Month field. The number of the pattern symbol letters follows the
> 742:      * same presentation, such as "number" or "text" as in the <a href="#patterns">Patterns for

I would reword as:
     * All pattern symbols are optional, and each pattern symbol represents a field,
     * for example, 'M' represents the Month field. The number of the pattern symbol letters follows the

src/java.base/share/classes/java/time/format/DateTimeFormatter.java line 753:

> 751:      * <p>
> 752:      * The locale is determined from the formatter. The formatter returned directly by
> 753:      * this method will use the {@link Locale#getDefault() default FORMAT locale}.

Editorial suggestion:
* this method uses the {https://github.com/link Locale#getDefault() default FORMAT locale}.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 231:

> 229: 
> 230:     /**
> 231:      * Gets the formatting pattern for the requested template for a locale and chronology.

"Returns" is more conventional (than "Gets; though it is consistent within the file)

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 246:

> 244:      * @param locale  the locale, non-null
> 245:      * @return the locale and Chronology specific formatting pattern
> 246:      * @throws IllegalArgumentException if {@code requestedTemplate} is invalid

The meaning "Invalid" should be clearly defined; repeating or refering to the template regex.

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 1485:

> 1483:      * <li>the {@code requestedTemplate} specified to this method
> 1484:      * <li>the {@code Locale} of the {@code DateTimeFormatter}
> 1485:      * <li>the {@code Chronology}, selecting the best available

Perhaps "best available" should be well defined, or just drop "best".
or ...
"of the {@code DateTimeFormatter} unless overridden"

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 1513:

> 1511:      * }
> 1512:      * All pattern symbols are optional, and each pattern symbol represents the field it is in,
> 1513:      * e.g., 'M' represents the Month field. The number of the pattern symbol letters follows the

Ditto above:
    * All pattern symbols are optional, and each pattern symbol represents the field,
     * for example, 'M' represents the Month field. The number of the pattern symbol letters follows the

src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 5148:

> 5146:             var useRequestedTemplate = requestedTemplate != null;
> 5147:             String key = chrono.getId() + '|' + locale.toString() + '|' +
> 5148:                     (useRequestedTemplate ? requestedTemplate : Objects.toString(dateStyle) + timeStyle);

The boolean `useRequestedTemplate` isn't necessary, replace with `requestedTemplate != null`.

src/java.base/share/classes/sun/text/spi/JavaTimeDateTimePatternProvider.java line 64:

> 62: 
> 63:     /**
> 64:      * Gets the formatting pattern for the requested template, calendarType, and locale.

"Returns" is more conventional.

src/java.base/share/classes/sun/util/locale/provider/JavaTimeDateTimePatternImpl.java line 69:

> 67:     public String getJavaTimeDateTimePattern(int timeStyle, int dateStyle, String calType, Locale locale) {
> 68:         LocaleResources lr = LocaleProviderAdapter.getResourceBundleBased().getLocaleResources(locale);
> 69:         return lr.getJavaTimeDateTimePattern(

Fold the parameters onto a single line.

src/java.base/share/classes/sun/util/locale/provider/LocaleResources.java line 690:

> 688:     private String resolveInputSkeleton(String type) {
> 689:         var regionToSkeletonMap = inputSkeletons.get(type);
> 690:         return regionToSkeletonMap.getOrDefault(locale.getLanguage() + "-" + locale.getCountry(),

This structure computes all the defaults even if the value isn't needed (because the value has to be passed to the `getOrDefault` method.  Perhaps performance isn't an issue.

-------------

PR: https://git.openjdk.java.net/jdk/pull/7340