6228794: java.text.ChoiceFormat pattern behavior is not well documented. by justin-curtis-lu · Pull Request #15392 · openjdk/jdk

justin-curtis-lu · 2023-08-22T19:18:35Z

Please review this PR and associated CSR which expands on the java.text.ChoiceFormat specification regarding its pattern.

j.text.ChoiceFormat provides an example pattern in the class description, but beyond that it does not specify any well-defined syntax for creating a pattern. In addition, methods related to the pattern String are under-specified.

The wording for getLimits() and getFormats() was also adjusted, as there are other ways to set the limits and formats beyond the constructor.

The pattern syntax may be easier to view -> https://cr.openjdk.org/~jlu/api/java.base/java/text/ChoiceFormat.html

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue
Change requires CSR request JDK-8314546 to be approved

Issues

JDK-6228794: java.text.ChoiceFormat pattern behavior is not well documented. (Bug - P4)
JDK-8314546: java.text.ChoiceFormat pattern behavior is not well documented. (CSR)

Reviewers

Naoto Sato (@naotoj - Reviewer) ⚠️ Review applies to 4103cf9b

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15392/head:pull/15392
$ git checkout pull/15392

Update a local copy of the PR:
$ git checkout pull/15392
$ git pull https://git.openjdk.org/jdk.git pull/15392/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15392

View PR using the GUI difftool:
$ git pr show -t 15392

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15392.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2023-08-22T19:19:27Z

👋 Welcome back jlu! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2023-08-22T19:20:33Z

@justin-curtis-lu The following labels will be automatically applied to this pull request:

core-libs
i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

mlbridge · 2023-08-22T19:46:17Z

Webrevs

openjdk · 2023-09-01T21:56:47Z

@justin-curtis-lu this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout JDK-6228794-cFmt-Doc
git fetch https://git.openjdk.org/jdk.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

naotoj · 2023-09-06T22:41:12Z

src/java.base/share/classes/java/text/ChoiceFormat.java

- * System.out.println("Format with 1.5 : " + fmt.format(1.5));
- * System.out.println("Format with 2 : " + fmt.format(2));
- * System.out.println("Format with 2.1 : " + fmt.format(2.1));
+ * // ...


Commenting out (with ellipsis) would not print the result below

I felt it was redundant to have both the long lines of println and output, so I commented out the println(s) except for the ones of interest, which are Double.NEGATIVE_INFINITY, Double.POSITIVE_INFINITY, and Double.NaN.

However, you're right that commenting it out would not print the below result. I have updated it so that the println and output lines are combined, to provide a more concise example.

naotoj · 2023-09-06T23:04:59Z

src/java.base/share/classes/java/text/ChoiceFormat.java

+ * <i>SubPattern:</i>
+ *         Limit Relation Format
+ * <i>Limit:</i>
+ *         {@code String} that can be parsed as a {@code double} / "&infin;" ({@code U+221E}) / "-&infin;" (-{@code U+221E}).


Could the first choice be more specific? Also, do SubPatterns need to be sorted with Limits?

Could the first choice be more specific?

Updated with a more syntactical definition. Please let me know if you think this is too specific.

Also, do SubPatterns need to be sorted with Limits?

If I am interpreting the question right, you're asking if a subPattern needs a limit to be valid. If that's the question, the answer is yes, ChoiceFormat is designed to have an equal amount of limits and formats (whether from the arrays, or the String pattern). But please let me know if I misinterpreted the question.

subpattern "xyz" where both limit and relation are omitted

jshell> var a = new ChoiceFormat("0#abc|xyz") a ==> java.text.ChoiceFormat@17863 jshell> a.getLimits() $21 ==> double[1] { 0.0 } jshell> a.getFormats() $22 ==> String[1] { "abc" }

subpattern "#xyz" where limit is omitted

jshell> var b = new ChoiceFormat("0#abc|#xyz") | Exception java.lang.IllegalArgumentException: Each interval must contain a number before a format

Please let me know if you think this is too specific.

I think the updated one is better.
For the second part, I meant:

jshell> new ChoiceFormat("0#def|-1#abc|xyz") | Exception java.lang.IllegalArgumentException: Incorrect order of intervals, must be in ascending order

This should be excluded from the pattern syntax

This should be excluded from the pattern syntax

I presume you meant that the syntax should include the required ascending order of limits. Updated to make this apparent, I also adjusted the syntax a little bit to be more accurate. The previous syntax could technically have an empty Number

naotoj · 2023-09-06T23:09:18Z

src/java.base/share/classes/java/text/ChoiceFormat.java


    /**
-     * Constructs with limits and corresponding formats based on the pattern.
+     * Constructs this ChoiceFormat with limits and corresponding formats


this -> a

naotoj · 2023-09-06T23:11:08Z

src/java.base/share/classes/java/text/ChoiceFormat.java


    /**
-     * Get the limits passed in the constructor.
+     * Get the limits of this ChoiceFormat.


@return can be applied

naotoj · 2023-09-06T23:11:36Z

src/java.base/share/classes/java/text/ChoiceFormat.java


    /**
-     * Get the formats passed in the constructor.
+     * Get the formats of this ChoiceFormat.


Fixed, as well as in the other instance, I have also updated https://cr.openjdk.org/~jlu/api/java.base/java/text/ChoiceFormat.html to reflect the changes. Thanks for the review

…equences

…wording, a choiceFormat does NOT have to be created with only the arrays constructor.

naotoj

LGTM, with a minor nit.

naotoj · 2023-09-08T20:21:56Z

src/java.base/share/classes/java/text/ChoiceFormat.java

 *         "#" / "&lt;" / "&le;"
 * <i>Format:</i>
- *         {@code String}
+ *         Any unicode characters except the <i>Relation</i> symbols


Nit: since characters in Java are of Unicode code points, no need to mention unicode here.

Thanks for the follow-up review. Fixed the nit, and updated the CSR to reflect all of the changes.

(I used the 'Unicode' wording because the other Format classes did, I will adjust it for the other classes in this issue

jddarcy · 2023-09-12T17:53:43Z

src/java.base/share/classes/java/text/ChoiceFormat.java

+ *
+ * <i>Note:The relation &le; is not equivalent to &lt;&equals;</i>
+ *
+ * <p>Below is an example of constructing a ChoiceFormat with a pattern:


FWIW, in other parts of core libs such as java.lang.Double, I've used a definition list <dl> with terms <dt> and definitions <dd> to represent such grammars.

openjdk · 2023-09-12T17:56:56Z

@justin-curtis-lu This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

6228794: java.text.ChoiceFormat pattern behavior is not well documented.

Reviewed-by: naoto

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 117 new commits pushed to the master branch:

ac2dfe4: 8313865: Always true condition in sun.nio.cs.CharsetMapping#readINDEXC2B
947dee7: 8316113: Infinite permission checking loop in java/net/spi/InetAddressResolverProvider/RuntimePermissionTest
1d702d2: 8313800: AArch64: SA stack walking code having trouble finding sender frame when invoking LambdaForms is involved
e084516: 8315968: Move java.util.Digits to jdk.internal.util and refactor to reduce duplication
d75d977: 8285447: StackWalker minimal batch size should be optimized for getCallerClass
fc3e826: 8314832: Few runtime/os tests ignore vm flags
347beb2: 8315998: Remove dead ClassLoaderDataGraphKlassIteratorStatic
50158f3: 8316002: Remove unnecessary seen_dead_loader in ClassLoaderDataGraph::do_unloading
6f2684b: 8315948: JDK-8315818 broke Xcomp on libgraal
8b4f9a8: 8315990: Amend problemlisted tests to proper position
... and 107 more: https://git.openjdk.org/jdk/compare/4f90abaf17716493bad740dcef76d49f16d69379...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

…n Number section

justin-curtis-lu · 2023-09-13T20:31:49Z

/integrate

openjdk · 2023-09-13T20:32:23Z

Going to push as commit ce93d27.
Since your change was applied there have been 137 commits pushed to the master branch:

3b0a6d2: 8314226: Series of colon-style fallthrough switch cases with guards compiled incorrectly
ff240a9: 8316087: Test SignedLoggerFinderTest.java is still failing
a731a24: 8315934: RISC-V: Disable conservative fences per vendor
b3dad24: 8316021: Serial: Remove unused Generation::post_compact
f9ab115: 8316050: Use hexadecimal encoding in MemorySegment::toString
f804f86: 8314612: TestUnorderedReduction.java fails with -XX:MaxVectorSize=32 and -XX:+AlignVector
f8df754: 8311207: Cleanup for Optimization for UUID.toString
fecd2fd: 8315898: Open source swing JMenu tests
bb6b3f2: 8315761: Open source few swing JList and JMenuBar tests
2d168c5: 8313202: MutexLocker should disallow null Mutexes
... and 127 more: https://git.openjdk.org/jdk/compare/4f90abaf17716493bad740dcef76d49f16d69379...master

Your commit was automatically rebased without conflicts.

openjdk · 2023-09-13T20:32:31Z

@justin-curtis-lu Pushed as commit ce93d27.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

justin-curtis-lu added 6 commits August 17, 2023 13:31

Init

4f99052

Adjust wording of getter methods

ea0e1e9

Revert the example changes to focus on the more important changes

d98d3a9

Remove leftover comments

c240af8

Adjust pattern definition

cddafe9

Clarify ≤

8b9a22d

openjdk bot added csr Pull request needs approved CSR before integration rfr Pull request is ready for review labels Aug 22, 2023

openjdk bot added core-libs core-libs-dev@openjdk.org i18n i18n-dev@openjdk.org labels Aug 22, 2023

justin-curtis-lu mentioned this pull request Aug 29, 2023

8314925: ChoiceFormat does not specify IllegalArgumentExceptions #15473

Closed

4 tasks

Drop throws tag, that change can go to JDK8314925

2ed9d4d

justin-curtis-lu changed the title ~~6228794: [Fmt-Ch] Doc: java.text.ChoiceFormat behavior is not well documented.~~ 6228794: java.text.ChoiceFormat pattern behavior is not well documented. Sep 1, 2023

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Sep 1, 2023

justin-curtis-lu added 2 commits September 1, 2023 15:06

Include Unicode sequence in pattern

5eace20

merge master and resolve conflicts

fd1cc83

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Sep 1, 2023

naotoj reviewed Sep 6, 2023

View reviewed changes

justin-curtis-lu added 5 commits September 7, 2023 10:18

Reflect Naoto's comments from 9/6

bbb99eb

Review: Make ascending format clear, improve syntax, remove unicode s…

e3dafe6

…equences

Make Format definition more consistent with other Format classes

629dd7d

Improve throws description for input that takes pattern

684e6c5

Make starting sentence consistent with other Format classes. Improve …

4103cf9

…wording, a choiceFormat does NOT have to be created with only the arrays constructor.

naotoj approved these changes Sep 8, 2023

View reviewed changes

Drop Unicode from definition, implied

216e5ac

jddarcy reviewed Sep 12, 2023

View reviewed changes

openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Sep 12, 2023

Use <d_> description tags to create syntax, include negative symbol i…

ff1be6a

…n Number section

openjdk bot added the integrated Pull request has been integrated label Sep 13, 2023

openjdk bot closed this Sep 13, 2023

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 13, 2023

Conversation

justin-curtis-lu commented Aug 22, 2023 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Aug 22, 2023

Uh oh!

openjdk bot commented Aug 22, 2023

Uh oh!

mlbridge bot commented Aug 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

openjdk bot commented Sep 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naotoj left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openjdk bot commented Sep 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

justin-curtis-lu commented Sep 13, 2023

Uh oh!

openjdk bot commented Sep 13, 2023

Uh oh!

openjdk bot commented Sep 13, 2023

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

justin-curtis-lu commented Aug 22, 2023 •

edited by openjdk bot

Loading

mlbridge bot commented Aug 22, 2023 •

edited

Loading

openjdk bot commented Sep 12, 2023 •

edited

Loading