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

Fallback to LC_MONETARY when formatting currency #1051

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

Conversation

afh
Copy link

@afh afh commented Dec 21, 2023

POSIX defines the LC_MONETARY category to be used for monetary formatting, hence babel could be more standards-compliant in this regard.

@afh
Copy link
Author

afh commented Jan 5, 2024

Friendly ping to @akx and @oprypin to get some eyes on this. Not sure if you are the right folks to ping, yet looking at other PRs it seems you would at least know who else to get involved 🙂

Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

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

This is technically okay, but we really should fall back to LC_NUMERIC if LC_MONETARY isn't set so as to match the old behavior for people who don't have LC_MONETARY set; right now default_locale would then fall back to LC_ALL, LANG, etc.

You'd need to change default_locale() to accept multiple "primary options" before those fallbacks.

Furthermore, previously, LC_MONETARY was ignored altogether, but now it will be used if set for money formatting, which could be a surprising change for people who have it set. This change will need to be thoroughly documented in the changelog.

* ``LC_NUMERIC``,
* ``LC_NUMERIC`` or ``LC_MONETARY`` for currency related functions,
Copy link
Member

Choose a reason for hiding this comment

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

This documentation doesn't seem to match the new behavior; it's not an "or" if the envvar is set. This should probably say

Suggested change
* ``LC_NUMERIC``,
* ``LC_NUMERIC`` or ``LC_MONETARY`` for currency related functions,
* ``LC_MONETARY`` for currency related functions,
* ``LC_NUMERIC`` for other numbers,

or something along those lines.

@afh
Copy link
Author

afh commented Feb 1, 2024

Thanks for the review and feedback, @akx, much appreciated. I'll look into addressing your comments and updating this PR accordingly. Not sure when I can get to it, but I'll try to make time soon.

@akx
Copy link
Member

akx commented Jul 18, 2024

@afh Do you have time to maybe look at this PR? Could get it merged for 2.16 :)

@afh
Copy link
Author

afh commented Jul 18, 2024

Thanks for reaching out, @akx 🙂
Unfortunately I cannot make the time in the foreseeable future to address your suggested improvements, as much as I wish I could 😕
I might be able to test, if that would be helpful and someone else would be willing to take on the implementation.

@akx
Copy link
Member

akx commented Jul 18, 2024

@afh No worries, thanks for replying! I can take this over, no problem.

@afh
Copy link
Author

afh commented Jul 18, 2024

Very much appreciated, @akx! 🙏 Do ping me if there's something that can be tested.

@akx akx self-assigned this Jul 23, 2024
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