Re: RFR: 8176706: Additional Date-Time Formats [v4]
On Fri, 11 Feb 2022 22:26:03 GMT, Roger Riggs 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/DateTimeFormatterBuilder.java > line 5103: > >> 5101: * @param timeStyle the time style to use, may be null >> 5102: */ >> 5103: LocalizedPrinterParser(FormatStyle dateStyle, FormatStyle >> timeStyle) { > > Can the constructors be `private`. > The combination of package protected and the style of caller doing the > validation makes me a bit nervous. > There should not be any callers outside of DateTimeFormatterBuilder. Changed to `private` so that it can only be instantiated within `DateTimeFormatterBuilder`. Same modifications are applied to other `DateTimePrinterParser` implementations. > src/java.base/share/classes/sun/text/spi/JavaTimeDateTimePatternProvider.java > line 79: > >> 77: public String getJavaTimeDateTimePattern(String requestedTemplate, >> String calType, Locale locale) { >> 78: // default implementation throws exception >> 79: throw new DateTimeException("Formatter is not available for the >> requested template: " + requestedTemplate); > > "Formatting **pattern** is not available"... Modified, as well as other minor corrections in the javadoc. - PR: https://git.openjdk.java.net/jdk/pull/7340
Re: RFR: 8176706: Additional Date-Time Formats [v5]
> 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/af3c0d62..399257d4 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=7340&range=04 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7340&range=03-04 Stats: 31 lines in 2 files changed: 3 ins; 0 del; 28 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 [v4]
On Fri, 11 Feb 2022 00:03:50 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: > > Addressing review comments Looks good. src/java.base/share/classes/java/time/format/DateTimeFormatterBuilder.java line 5103: > 5101: * @param timeStyle the time style to use, may be null > 5102: */ > 5103: LocalizedPrinterParser(FormatStyle dateStyle, FormatStyle > timeStyle) { Can the constructors be `private`. The combination of package protected and the style of caller doing the validation makes me a bit nervous. There should not be any callers outside of DateTimeFormatterBuilder. src/java.base/share/classes/sun/text/spi/JavaTimeDateTimePatternProvider.java line 66: > 64: * Returns the formatting pattern for the requested template, > calendarType, and locale. > 65: * Concrete implementation of this method will retrieve > 66: * a java.time specific pattern from selected Locale Provider. editorial: "from **the** selected Locale"... src/java.base/share/classes/sun/text/spi/JavaTimeDateTimePatternProvider.java line 79: > 77: public String getJavaTimeDateTimePattern(String requestedTemplate, > String calType, Locale locale) { > 78: // default implementation throws exception > 79: throw new DateTimeException("Formatter is not available for the > requested template: " + requestedTemplate); "Formatting **pattern** is not available"... - Marked as reviewed by rriggs (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/7340
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&pr=7340&range=03 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7340&range=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&pr=7340&range=02 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7340&range=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&pr=7340&range=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=7340&range=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&pr=7340&range=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