Re: RFR: 8176706: Additional Date-Time Formats [v3]
On Thu, 10 Feb 2022 22:20:48 GMT, Roger Riggs wrote:
>> 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/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.
Indeed, yes. I thought this was simple enough, and as you said, not
performance-critical.
-
PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats [v4]
> 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: Addressing review comments - Changes: - all: https://git.openjdk.java.net/jdk/pull/7340/files - new: https://git.openjdk.java.net/jdk/pull/7340/files/41408ff3..af3c0d62 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7340=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7340=02-03 Stats: 33 lines in 5 files changed: 1 ins; 2 del; 30 mod Patch: https://git.openjdk.java.net/jdk/pull/7340.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7340/head:pull/7340 PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats [v3]
On Tue, 8 Feb 2022 19:08:45 GMT, Naoto Sato 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 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: *
> 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: * the {@code requestedTemplate} specified to this method
> 1484: * the {@code Locale} of the {@code DateTimeFormatter}
> 1485: * 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
Re: RFR: 8176706: Additional Date-Time Formats [v3]
On Tue, 8 Feb 2022 19:08:45 GMT, Naoto Sato 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 make/jdk/src/classes/build/tools/cldrconverter/Bundle.java line 113: > 111: // DateFormatItem prefix > 112: final static String DATEFORMATITEM_KEY_PREFIX = "DateFormatItem."; > 113: final static String DATEFORMATITEM_INPUT_REGIONS_PREFIX = > "DateFormatItemInputRegions."; The canonical order of modifiers is "static final". - PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats [v3]
On Tue, 8 Feb 2022 19:08:45 GMT, Naoto Sato 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 Marked as reviewed by joehw (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats [v3]
> 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 - Changes: - all: https://git.openjdk.java.net/jdk/pull/7340/files - new: https://git.openjdk.java.net/jdk/pull/7340/files/059132f5..41408ff3 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7340=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7340=01-02 Stats: 3 lines in 1 file changed: 1 ins; 0 del; 2 mod Patch: https://git.openjdk.java.net/jdk/pull/7340.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7340/head:pull/7340 PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats [v2]
On Mon, 7 Feb 2022 21:22:12 GMT, Roger Riggs wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Modified per suggestions on the PR
>
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 5162:
>
>> 5160:
>> 5161: formatter = new
>> DateTimeFormatterBuilder().appendPattern(pattern).toFormatter(locale);
>> 5162: DateTimeFormatter old =
>> FORMATTER_CACHE.putIfAbsent(key, formatter);
>
> .computeIfAbsent(key, () -> {}) might be a bit cleaner/clearer here and
> avoid the race a few lines of code below. (a slight improvement in old code).
Good point. Changed to `computeIfAbsent()`.
-
PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats [v2]
On Tue, 8 Feb 2022 00:39:04 GMT, Joe Wang wrote:
>> Naoto Sato has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Modified per suggestions on the PR
>
> src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java
> line 254:
>
>> 252: public static String getLocalizedDateTimePattern(String
>> requestedTemplate,
>> 253: Chronology chrono,
>> Locale locale) {
>> 254: Objects.requireNonNull(locale, "requestedTemplate");
>
> Typo, "locale" should have been requestedTemplate.
Good catch!
> src/java.base/share/classes/sun/util/locale/provider/JavaTimeDateTimePatternImpl.java
> line 77:
>
>> 75: LocaleProviderAdapter lpa =
>> LocaleProviderAdapter.getResourceBundleBased();
>> 76: // CLDR's 'u'/'U' are not supported in the JDK. Replace them
>> with 'y' instead
>> 77: final var modifiedSkeleton =
>> requestedTemplate.replaceAll("[uU]", "y");
>
> Seems to me requestedTemplate needs to be validated when it gets passed to
> getLocalizedDateTimePattern, similar as to appendLocalized
This was a left over which should be removed. In fact, CLDR specific pattern
symbols should not be accepted as the requested template. Removed the
substitutions. As to the validation, it will be performed in the following
`LocaleResources.getLocalizedPattern()` method.
-
PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats [v2]
> 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: Modified per suggestions on the PR - Changes: - all: https://git.openjdk.java.net/jdk/pull/7340/files - new: https://git.openjdk.java.net/jdk/pull/7340/files/f9311dce..059132f5 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=7340=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=7340=00-01 Stats: 80 lines in 4 files changed: 23 ins; 37 del; 20 mod Patch: https://git.openjdk.java.net/jdk/pull/7340.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7340/head:pull/7340 PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats
On Thu, 3 Feb 2022 23:29:54 GMT, Naoto Sato 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
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
254:
> 252: public static String getLocalizedDateTimePattern(String
> requestedTemplate,
> 253: Chronology chrono,
> Locale locale) {
> 254: Objects.requireNonNull(locale, "requestedTemplate");
Typo, "locale" should have been requestedTemplate.
src/java.base/share/classes/sun/util/locale/provider/JavaTimeDateTimePatternImpl.java
line 77:
> 75: LocaleProviderAdapter lpa =
> LocaleProviderAdapter.getResourceBundleBased();
> 76: // CLDR's 'u'/'U' are not supported in the JDK. Replace them with
> 'y' instead
> 77: final var modifiedSkeleton = requestedTemplate.replaceAll("[uU]",
> "y");
Seems to me requestedTemplate needs to be validated when it gets passed to
getLocalizedDateTimePattern, similar as to appendLocalized
-
PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats
On Thu, 3 Feb 2022 23:29:54 GMT, Naoto Sato 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
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5115:
> 5113:
> 5114: private LocalizedPrinterParser(FormatStyle dateStyle,
> FormatStyle timeStyle, String requestedTemplate) {
> 5115: this.dateStyle = dateStyle;
Drop this constructor; it opens up the possibility of ambiguity between the
modes.
Keep only the two constructors with disjoint checking and initialization.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5151:
> 5149: */
> 5150: private DateTimeFormatter formatter(Locale locale, Chronology
> chrono) {
> 5151: var dtStyle = dateStyle !=null || timeStyle != null;
Switch to using requestedTemplate == null or != null as the discriminator.
src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line
5162:
> 5160:
> 5161: formatter = new
> DateTimeFormatterBuilder().appendPattern(pattern).toFormatter(locale);
> 5162: DateTimeFormatter old =
> FORMATTER_CACHE.putIfAbsent(key, formatter);
.computeIfAbsent(key, () -> {}) might be a bit cleaner/clearer here and
avoid the race a few lines of code below. (a slight improvement in old code).
src/java.base/share/classes/sun/util/locale/provider/LocaleResources.java line
606:
> 604: var matcher = VALID_SKELETON_PATTERN.matcher(skeleton);
> 605: if (!matcher.matches()) {
> 606: throw new IllegalArgumentException("Requested template is
> invalid: " + requestedTemplate);
The substitutions are not going to be visible in the exception message.
That might make it harder to identify and fix the cause of the exception.
Perhaps the message can make it clear that the matching was done after
substitutions and show the 'skeleton'.
-
PR: https://git.openjdk.java.net/jdk/pull/7340
RFR: 8176706: Additional Date-Time Formats
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 - Commit messages: - Removed trailing space - Merge branch 'master' into skeleton - Tidying up - Some more tests - Brought back the part that was mistakenly removed - Merge branch 'master' into skeleton - Reverted IllegalArgumentException back - Further cleanups - Removed exceptions from ofLocalizedPattern() method, as they are lazily thrown on format() - Cleanup - ... and 19 more: https://git.openjdk.java.net/jdk/compare/cda9c301...f9311dce Changes: https://git.openjdk.java.net/jdk/pull/7340/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=7340=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8176706 Stats: 777 lines in 12 files changed: 733 ins; 9 del; 35 mod Patch: https://git.openjdk.java.net/jdk/pull/7340.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/7340/head:pull/7340 PR: https://git.openjdk.java.net/jdk/pull/7340
