Improve correctness of POSIX locale conversion #6575
Conversation
Addresses the first half of unicode-org#6573: unicode-org#6573
Only appends the `-posix` variant if the locale is "C" or "POSIX". Should address the second half of unicode-org#6573: unicode-org#6573
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback. |
Thanks to @hsivonen for linking to the documentation!
| /// ); | ||
| /// # Ok(()) | ||
| /// # } | ||
| /// ``` | ||
| pub fn try_convert_lossy(&self) -> Result<Locale, ParseError> { | ||
| let mut extensions = Extensions::new(); | ||
| let mut script = None; | ||
| let mut variants = Vec::new(); |
There was a problem hiding this comment.
the comment in line 310 is obsolete
| /// This is based on GNU libc's `intl/locale.alias` file, with some manual processing | ||
| /// to remove duplicates. The original file is available at: | ||
| /// to remove duplicates. The notable exception is that the default `C`/`POSIX` locales | ||
| /// map to `en-US`. The original file is available at: |
There was a problem hiding this comment.
| /// map to `en-US`. The original file is available at: | |
| /// map to `en-US-posix`. The original file is available at: |
| /// This is based on GNU libc's `intl/locale.alias` file, with some manual processing | ||
| /// to remove duplicates. The original file is available at: | ||
| /// to remove duplicates. The notable exception is that the default `C`/`POSIX` locales |
There was a problem hiding this comment.
| /// to remove duplicates. The notable exception is that the default `C`/`POSIX` locales | |
| /// to remove duplicates. The notable addition is that the default `C`/`POSIX` locales |
| "swedish" => Some((language!("sv"), Some(region!("SE")))), | ||
| "thai" => Some((language!("th"), Some(region!("TH")))), | ||
| "turkish" => Some((language!("tr"), Some(region!("TR")))), | ||
| match posix_locale { |
There was a problem hiding this comment.
Please remove this whole match block to remove the language name checks and unify the "C" and "POSIX" case handling with the block below where you currently append -posix.
The X11 (see https://www.x.org/releases/X11R6.9.0/src-single/X11R6.9.0-src.tar.bz2 ) language name aliases are obsolete, so we shouldn't carry the data or spend time matching them. They resolved to legacy-encoding locales and present-day distros generate UTF-8 locales, so even on the libc-level the aliases don't resolve to an available locale and result in C.
Addressing #6573, this PR implements the requested changes to how
env_preferencesconverts POSIX locales to ICU4X locales:C/POSIXlocale now maps toen-US-posixinstead ofund-posix-posixvariant addedThanks to @hsivonen for clarifying how it should be done! Please let me know if there's anything I can do to help in the review process :)