On Tue, 8 Aug 2023 17:19:55 GMT, Naoto Sato <na...@openjdk.org> wrote:
>> Introducing a new formatting class for locale-dependent list patterns. The >> class is to provide the functionality from the Unicode Consortium's LDML >> specification for [list >> patterns](https://www.unicode.org/reports/tr35/tr35-general.html#ListPatterns). >> For example, given a list of String as "Monday", "Wednesday", "Friday", its >> `format` method would produce "Monday, Wednesday, and Friday" in US English. >> A CSR has also been drafted, and its draft javadoc can be viewed here: >> https://cr.openjdk.org/~naoto/JDK-8041488-ListPatterns-PR/api.00/java.base/java/text/ListFormat.html > > Naoto Sato has updated the pull request with a new target base due to a merge > or a rebase. The incremental webrev excludes the unrelated changes brought in > by the merge/rebase. The pull request contains five additional commits since > the last revision: > > - Merge branch 'master' into JDK-8041488-ListPatterns-PR > - parsePosition update > - Wording fixes > - Added test > - Initial commit src/java.base/share/classes/java/text/ListFormat.java line 199: > 197: * @param locale Locale to be used, not null > 198: * @param type type of the list format. One of STANDARD/OR/UNIT, not > null > 199: * @param style style of the list format. One of FULL/SHORT/NARROW, > not null It would be ease navigation in the javadoc if there was a link to the ListFormat.Type and ListFormat.Style classes, perhaps Locale also. Use the normal prose form STANDARD, OR, or UNIT, instead of the "/" shorthand. Here and throughout the javadoc. Ditto FULL, SHORT, or NARROW. Also use Locale, Type, and Style instead of the shorthand. src/java.base/share/classes/java/text/ListFormat.java line 213: > 211: /** > 212: * {@return the list format for the specified patterns} > 213: * <p> LDML includes a section on the "Gender of Lists"; If later support was desired, is this API extensible to include that? https://www.unicode.org/reports/tr35/tr35-general.html#List_Gender src/java.base/share/classes/java/text/ListFormat.java line 234: > 232: * If parsing of the pattern string for start/middle/end fails, it > throws an > 233: * {@code IllegalArgumentException}. If two/three pattern string is > empty, or > 234: * fails on parsing, it falls back to I don't see code to try to parse the TWO or THREE patterns, where it could detect incorrectly formatted (user supplied) patterns. If I understand correctly, a bad TWO or THREE pattern would not be detected until it is used in format or parse. src/java.base/share/classes/java/text/ListFormat.java line 350: > 348: * {@code "a", "b", "c"}. > 349: * > 350: * @param source the String to parse, not null. String does not need to be capitalized. src/java.base/share/classes/java/text/ListFormat.java line 365: > 363: > 364: /** > 365: * Parses text from a string to produce a list of {@code String}s. No need to capitalize String if list is not capitalized. (or be consistent) src/java.base/share/classes/java/text/ListFormat.java line 428: > 426: return Arrays.asList(objs); > 427: } > 428: throw new InternalError("MessageFormat.parseObject() should > return Object[]"); The error message is inconsistent with line 426, returning a List<String>. src/java.base/share/classes/java/text/ListFormat.java line 442: > 440: > 441: /** > 442: * {@inheritDoc} The javadoc from Object.equals doesn't give any information about what constituted equals for a ListFormat. Namely, that the locale and patterns are equal. src/java.base/share/classes/java/text/ListFormat.java line 480: > 478: var len = input.length; > 479: return switch (len) { > 480: case 0 -> throw new IllegalArgumentException("There should > at least be one input string"); It is unfortunate that LDML did not consider the case of an empty list. For example, to supply "NONE" or "n/a" or similar. The developer will need to deal with that separately. ------------- PR Review Comment: https://git.openjdk.org/jdk/pull/15130#discussion_r1287548046 PR Review Comment: https://git.openjdk.org/jdk/pull/15130#discussion_r1287623678 PR Review Comment: https://git.openjdk.org/jdk/pull/15130#discussion_r1287564928 PR Review Comment: https://git.openjdk.org/jdk/pull/15130#discussion_r1287624574 PR Review Comment: https://git.openjdk.org/jdk/pull/15130#discussion_r1287626211 PR Review Comment: https://git.openjdk.org/jdk/pull/15130#discussion_r1287627985 PR Review Comment: https://git.openjdk.org/jdk/pull/15130#discussion_r1287630232 PR Review Comment: https://git.openjdk.org/jdk/pull/15130#discussion_r1287622614
