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

[20+] fix warning 'The constructor Locale is deprecated since version 20' #1406

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jukzi
Copy link
Contributor

@jukzi jukzi commented Jun 4, 2024

No description provided.

Copy link
Contributor

github-actions bot commented Jun 4, 2024

Test Results

 1 728 files   - 6   1 728 suites   - 6   1h 28m 38s ⏱️ + 3m 49s
 3 966 tests  - 1   3 944 ✅  - 1   22 💤 ±0  0 ❌ ±0 
12 495 runs   - 3  12 334 ✅  - 3  161 💤 ±0  0 ❌ ±0 

Results for commit 2a1bf40. ± Comparison against base commit fda6401.

This pull request removes 1 test.
AutomatedConfiguratorSuite FeatureEntryTests ‑ testLicenseUrl

♻️ This comment has been updated with latest results.

@jukzi jukzi changed the title fix warning 'The constructor Locale is deprecated since version 20' [20+] fix warning 'The constructor Locale is deprecated since version 20' Jun 4, 2024
@jukzi
Copy link
Contributor Author

jukzi commented Jun 13, 2024

i guess this has to wait until BREE>=20

public static Locale getDefaultLocale() {
String nl = getNL();
// sanity test
if (nl == null)
return Locale.getDefault();


// break the string into tokens to get the Locale object
Copy link
Contributor

@mihnita mihnita Aug 12, 2024

Choose a reason for hiding this comment

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

I would suggest a more correct way of creating a locale:
Locale.forLanguageTag(localeId.replace('_', '-'))

See here for some details:
https://errorprone.info/bugpattern/UnsafeLocaleUsage

Should never use the Locale.toString(), the Locale constructor and Locale.of now (that I didn't know about).

  • They don't support standard BCP 47 locale identifiers, including very common ones that have scripts. For example zh-Hant, or sr-Latn-RS.
  • No Unicode extensions (en-US-u-hc-h23, or ar-u-nu-latn)
  • No "real variant" (for example de-CH-1996 or ca_ES_VALENCIA)

https://www.rfc-editor.org/rfc/bcp/bcp47.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • They don't support standard BCP 47 locale identifiers

Hi @mihnita, thanks for the qualified comment. However introducing BCP 47 would change the documented behavior. That is not what this PR is about. As far as i understand this code reflects the Localization defined by OSGi in https://docs.osgi.org/specification/osgi.core/8.0.0/framework.module.html#framework.module.localization chapter 3.11, which defines only language, country, variant
See also https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/72743b4569d5f8d522fd93f4a8875077d32e1fc3/eclipse.platform.common/bundles/org.eclipse.platform.doc.isv/reference/misc/runtime-options.html#L721 ff
NL values should follow the standard Java locale naming conventions. i could not find any reference that BCP 47 encoding can or should be used or that anybody missed a certain local feature beside language, country, variant.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, if this applies to resource bundles.

It is really unfortunate that OSGi went with language+region+variant.
This system cannot represent certain locales that are currently in use. For example sr-Cyrl-RS vs sr-Latn-RS (Serbian in Cyrillic / Latin script, in Serbia).
I was not aware of this, and I am quite disappointed to see it. :-(

NL values should follow the standard Java locale naming conventions. i could not find any reference that BCP 47 encoding can or should be used or that anybody missed a certain local feature beside language, country, variant.

I think the Java locales are very much about BCP 47 these days:

The Locale class implements IETF BCP 47 which is composed of RFC 4647 "Matching of Language Tags" and RFC 5646 "Tags for Identifying Languages" with support for the LDML (UTS#35, "Unicode Locale Data Markup Language") BCP 47-compatible extensions for locale data exchange.
...
A Locale object logically consists of the fields described below:
... language, script, country (region), variant, extensions

...
Because of these issues, it is recommended that clients migrate away from constructing non-conforming locales and use the forLanguageTag and Locale.Builder APIs instead. Clients desiring a string representation of the complete locale can then always rely on toLanguageTag for this purpose.
...
Java's default resource bundle lookup mechanism also implements this mapping, so that resources can be named using either convention, see ResourceBundle.Control.
...
https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/Locale.html

Note especially "it is recommended that clients migrate away from constructing non-conforming locales and use the forLanguageTag and Locale.Builder APIs instead."

I have no idea why Java would add a factory method (of) that takes the legacy style lang + region + variant.

And why, even if the Java Locale says "resources can be named using either convention", OSGi went "the legacy way" :-(

And if it is indeed about OSGi resources, there might be problems in some cases, as there is no way to cleanly map between BCP 47 and lang+region+variant.
For example if my default locale is sr-Latn-RS, what will be the lang+region+variant equivalent? (see the "Compatibility" section in the Locale documentation.)


TLDR: if this is strictly about resources, and more precise about OSGi resources, then you change is OK as is.
Maybe add a comment.

If it is about something else, then probably BCP 47 is still the best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OSGi went "the legacy way"

I am sure OSGI is open for improvements. If there is any backward compatible way to support both lang+region+variant and BCP47 we could also just apply it here.

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