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

fix: get_locale follows posix standard #1264

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mirhahn
Copy link

@mirhahn mirhahn commented Dec 31, 2024

Description

This change brings the behavior of the get_locale function in the eww_shared_util in line with the POSIX standard. Previously, the function relied exclusively on the LC_TIME environment variable. However, this variable is almost never set because it is technically only needed if different locales are used for different categories.

The changed function uses the environment variables LC_ALL, LC_TIME, and LANG in this specific order of preference. This order is prescribed by Section 8.2 of IEEE Std 1003.1-2008 as mandatory for implementations of that version of the POSIX standard, so this should blend in well in all environments that follow that standard.

Additional Notes

I have classified this as a non-breaking change. It technically alters behavior on systems where both LC_ALL and LC_TIME are set because LC_ALL will now take precedence, but there are no API changes.

The precise method of locale determination does not seem to be mentioned in the documentation, so I did not change anything there.

Checklist

Please make sure you can check all the boxes that apply to this PR.

  • All widgets I've added are correctly documented.
  • I added my changes to CHANGELOG.md, if appropriate.
  • The documentation in the docs/content/main directory has been adjusted to reflect my changes.
  • I used cargo fmt to automatically format all code before committing

Copy link
Contributor

@w-lfchen w-lfchen left a comment

Choose a reason for hiding this comment

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

after having a quick read through the linked specification, this seems correct. thank you for pointing this out!

this pr made me review this function again and tbh i'm not really happy with it, here's an alternative body i find much better (i wrote this in about 3 minutes, there may be room for improvements):

    var("LC_ALL")
        .or_else(|_| var("LC_TIME"))
        .or_else(|_| var("LANG"))
        .map_or(Locale::POSIX, |v| v.split(".").next().and_then(|x| x.try_into().ok()).unwrap_or_default())

there's no need to rewrite the whole function here, though i'd love to see it happen

crates/eww_shared_util/src/locale.rs Outdated Show resolved Hide 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.

2 participants