-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
JDK-6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns #17856
JDK-6801704: ChoiceFormat::applyPattern inconsistency for invalid patterns #17856
Conversation
👋 Welcome back jlu! A progress list of the required criteria for merging this PR into |
@justin-curtis-lu The following labels will be automatically applied to this pull request:
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. |
Webrevs
|
@@ -230,6 +230,12 @@ public class ChoiceFormat extends NumberFormat { | |||
* Patterns} section. Unlike {@link #setChoices(double[], String[])} this | |||
* method will throw an {@code IllegalArgumentException} if the {@code | |||
* limits} are not in ascending order. | |||
* | |||
* @implNote Given an incorrect pattern, this implementation may either | |||
* throw an exception or succeed and discard the incorrect |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An explanation for the exception may be helpful, either enumerating possible exceptions or simply a RuntimeException
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think initially since it was an implSpec
tag, I didn't want to over-detail the inconsistent behavior as specification. As an implNote
tag it seems much better to give further explanation. I added details for the two possible exceptions. Let me know if you think I should go into further detail on when a pattern may be discarded if you think that is necessary.
* @implNote Given an incorrect pattern, this implementation may either | ||
* throw an exception or succeed and discard the incorrect | ||
* portion. Discarding the incorrect portion may result in a ChoiceFormat | ||
* with empty {@code limits} and {@code choices}. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formats
instead of choices
? A choice
means a limit
+format
to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, should definitely be formats
here, thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for the changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
* ##patterns Patterns} section. | ||
* based on the pattern. The syntax and error related caveats for the | ||
* ChoiceFormat pattern can be found in the {@linkplain ##patterns Patterns} | ||
* section. Unlike {@link #ChoiceFormat(double[], String[])} this method will |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: constructor
better suited than method
.
@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:
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 240 new commits pushed to the
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 |
/integrate |
Going to push as commit b665fe3.
Your commit was automatically rebased without conflicts. |
@justin-curtis-lu Pushed as commit b665fe3. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
Please review this PR and CSR which defines the behavior for creating ChoiceFormats with incorrect patterns. The wording is added to both the ChoiceFormat constructor and ChoiceFormat::applyPattern method.
While ideally the inconsistent behavior itself could be fixed, this behavior has been long-standing for 20+ years and the benefit of consistent error handling does not outweigh the risk of breaking applications that may be relying on the "expected" incorrect behavior.
Examples of the range of behavior, (all examples violate the pattern syntax defined in the class description)
Progress
Issues
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/17856/head:pull/17856
$ git checkout pull/17856
Update a local copy of the PR:
$ git checkout pull/17856
$ git pull https://git.openjdk.org/jdk.git pull/17856/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 17856
View PR using the GUI difftool:
$ git pr show -t 17856
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/17856.diff
Webrev
Link to Webrev Comment