-
Notifications
You must be signed in to change notification settings - Fork 6.2k
8355522: Remove the java.locale.useOldISOCodes system property
#26419
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
Conversation
|
👋 Welcome back naoto! A progress list of the required criteria for merging this PR into |
|
@naotoj 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 8 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 |
Webrevs
|
justin-curtis-lu
left a comment
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.
Should we also remove the test method, ModuleTestUtil.runModuleWithLegacyCode which passes the now defunct property to the process.
| // Ensure java.locale.useOldISOCodes is only interpreted at runtime startup | ||
| // Should have no effect | ||
| // Ensure java.locale.useOldISOCodes should have no effect | ||
| System.setProperty("java.locale.useOldISOCodes", "false"); |
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.
IMO, it seems weird to keep this line in the test, even if it has no effect. The original goal was to ensure the property only had impact when set during startup. The current test is no longer concerned with that (since the property no longer performs any mapping).
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.
Right. I re-purposed the test but as you mentioned, the line is confusing. Removed.
I thought about that, but decided to leave it as it is, just to make sure everything works as before. For the same reason, I did not remove the |
| variant.intern())); | ||
| } | ||
|
|
||
| public static String convertOldISOCodes(String language) { |
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.
It was there before this change, but above on line 166 I think we should update the outdated comment,
// JDK uses deprecated ISO639.1 language codes for he, yi and id
justin-curtis-lu
left a comment
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
| * <p>Locale's constructors have always converted three language codes to | ||
| * their earlier, obsoleted forms: {@code he} maps to {@code iw}, | ||
| * {@code yi} maps to {@code ji}, and {@code id} maps to | ||
| * {@code in}. Since Java SE 17, this is no longer the case. Each |
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.
* <p>Locale's constructors have always converted three language codes to
* their earlier, obsoleted forms: {@code he} maps to {@code iw},
* {@code yi} maps to {@code ji}, and {@code id} maps to
* {@code in}. Since Java SE 17, this is no longer the case.
This history was relevant when the property existed. Since this is no longer the case, and we're quite a few releases away from 17, can we also remove this wording as well. Users on 26 should only be concerned with the "old" to "modern" mapping.
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.
Good point. Modified the unnecesarry history.
|
@naotoj This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
/keepalive |
|
@naotoj The pull request is being re-evaluated and the inactivity timeout has been reset. |
|
@naotoj This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
@naotoj This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the |
|
This PR is intended for JDK27. Will reopen it when the time comes |
|
/open |
|
@naotoj This pull request is now open |
justin-curtis-lu
left a comment
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.
Now that PR is open again, did another take and looks good as before. Minor test comments.
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.
Since we are using JUnit, these assertions would be cleaner as assertEquals.
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.
In this test file, there are other locations that use fail. Although it would be preferred to replace them with assertEquals, probably better be cleaned up with a separate issue.
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.
Here and below, I think these errors should be including the old language in the fail message, not the new language. i.e. hebrewOld.getLanguage instead of hebrewNew.getLanguage.
Co-authored-by: Andrey Turbanov <turbanoff@gmail.com>
|
Thanks for the reviews! |
|
Going to push as commit b2daf9d.
Your commit was automatically rebased without conflicts. |
This PR removes the system property deprecated in JDK 25. If the property is specified at runtime, a warning will be emitted at startup to inform the user that the value is ignored. A corresponding CSR has been drafted as well
Progress
Issues
java.locale.useOldISOCodessystem property (Bug - P4)java.locale.useOldISOCodessystem property (CSR)Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/26419/head:pull/26419$ git checkout pull/26419Update a local copy of the PR:
$ git checkout pull/26419$ git pull https://git.openjdk.org/jdk.git pull/26419/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 26419View PR using the GUI difftool:
$ git pr show -t 26419Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/26419.diff
Using Webrev
Link to Webrev Comment