Skip to content
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

ICU-22919 Enable CI with JDK 21 and fix the json parsing #3214

Merged
merged 1 commit into from
Sep 25, 2024

Conversation

mihnita
Copy link
Contributor

@mihnita mihnita commented Sep 25, 2024

At the code of the problem is a change in how Java parses strings to Double:

System.out.println(Double.valueOf("1234567890123456789"));

The results are:
jdk 17 : 1.23456789012345677E18
jdk 21 : 1.2345678901234568E18

The json parser was seeing a number and parsed it to a Double.
With the change the json parser sees a string and passes that on to our test suite, and the string is parsed / interpreted by our code.

So we just took control from the json parser.

Java supports a long value = 1234567890123456789L, there is no rounding, and the result from MessageFormatter is "1,234,567,890,123,456,789", as one would expect.

So the current fix does not need to change any code in MessageFormatter.

It only bypasses the json parsing of numbers and informs the test code on how to parse the string value.

Before (json parser in control):
value: 1234567890123456789
After (our test suite in control):
value: {"decimal": "1234567890123456789"}

Checklist
  • Required: Issue filed: https://unicode-org.atlassian.net/browse/ICU-22919
  • Required: The PR title must be prefixed with a JIRA Issue number.
  • Required: The PR description must include the link to the Jira Issue, for example by completing the URL in the first checklist item
  • Required: Each commit message must be prefixed with a JIRA Issue number.
  • Issue accepted (done by Technical Committee after discussion)
  • Tests included, if applicable
  • API docs and/or User Guide docs changed or added, if applicable

@mihnita mihnita changed the title ICU-22919 Enable CI with JDK 21 and fix the json parsing DRAFT: ICU-22919 Enable CI with JDK 21 and fix the json parsing Sep 25, 2024
@mihnita mihnita marked this pull request as draft September 25, 2024 00:44
Copy link
Member

@cjchapman cjchapman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that all tests pass and I get BUILD SUCCESS when I run mvn verify locally with these changes. Thanks!

@markusicu markusicu self-assigned this Sep 25, 2024
@mihnita mihnita marked this pull request as ready for review September 25, 2024 16:21
@mihnita mihnita changed the title DRAFT: ICU-22919 Enable CI with JDK 21 and fix the json parsing ICU-22919 Enable CI with JDK 21 and fix the json parsing Sep 25, 2024
@mihnita
Copy link
Contributor Author

mihnita commented Sep 25, 2024

I confirmed that all tests pass and I get BUILD SUCCESS when I run mvn verify locally with these changes. Thanks!

Thank you!
And thanks for finding it :-)

@mihnita mihnita merged commit 8ce61b1 into unicode-org:main Sep 25, 2024
102 checks passed
@mihnita mihnita deleted the mihai_mf2_jdk21_json branch September 25, 2024 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants