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

Parse system locales in env_preferences #6158

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Finchiedev
Copy link

As per the discussion in #6028, I have created a POSIX locale parser/converter, currently hidden in the private env_preferences::parse module. This is meant to change at some point while the PR is being drafted, especially once I add support for other platforms. Once more platforms are supported, I would also like to implement universal and platform-specific APIs, as per this comment by @zbraniecki.

My current thinking on code structure is to have some distinction between platform fetch and parse code (either using modules or files), but please let me know if all platform logic should just be kept in the same file.

Of course, feedback on the code itself would be very much appreciated!

As per the discussion in unicode-org#6028, I have implemented POSIX locale parsing
functionality, and a `try_convert_lossy()` function to attempt converting
to a `icu_locale::Locale`. This code is currently in the private `parse`
module, as a temporary solution while support for other platforms is
added and the code structure is finalized.
@CLAassistant
Copy link

CLAassistant commented Feb 19, 2025

CLA assistant check
All committers have signed the CLA.

Basic testing of each edge case, error case, along with end-to-end
tests using some common POSIX locales.
Copy link
Member

@robertbastian robertbastian left a comment

Choose a reason for hiding this comment

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

code looks great, @zbraniecki can you also take a look

@robertbastian robertbastian marked this pull request as ready for review February 19, 2025 11:07
@robertbastian robertbastian requested a review from a team as a code owner February 19, 2025 11:07
@robertbastian robertbastian marked this pull request as draft February 19, 2025 11:07
@robertbastian
Copy link
Member

(whoops wrong button)

Finchiedev added a commit to Finchiedev/icu4x that referenced this pull request Feb 19, 2025
@Finchiedev
Copy link
Author

Thanks very much for your time and feedback @robertbastian, I have implemented all the suggested changes and marked them as resolved.

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