On Tue, 8 Aug 2023 17:19:55 GMT, Naoto Sato <naoto at 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