Skip to content

Fallback to LC_MONETARY when formatting currency#1051

Closed
afh wants to merge 1 commit into
python-babel:masterfrom
afh:currency_lc_monetary
Closed

Fallback to LC_MONETARY when formatting currency#1051
afh wants to merge 1 commit into
python-babel:masterfrom
afh:currency_lc_monetary

Conversation

@afh

@afh afh commented Dec 21, 2023

Copy link
Copy Markdown
Contributor

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

@afh

afh commented Jan 5, 2024

Copy link
Copy Markdown
Contributor Author

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 🙂

@akx akx left a comment

Copy link
Copy Markdown
Member

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.

Comment thread babel/numbers.py
following environment variables, in that order:

* ``LC_NUMERIC``,
* ``LC_NUMERIC`` or ``LC_MONETARY`` for currency related functions,

Copy link
Copy Markdown
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

afh commented Feb 1, 2024

Copy link
Copy Markdown
Contributor Author

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

akx commented Jul 18, 2024

Copy link
Copy Markdown
Member

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

@afh

afh commented Jul 18, 2024

Copy link
Copy Markdown
Contributor Author

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

akx commented Jul 18, 2024

Copy link
Copy Markdown
Member

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

@afh

afh commented Jul 18, 2024

Copy link
Copy Markdown
Contributor Author

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

@akx akx self-assigned this Jul 23, 2024
@akx akx closed this in #1173 Jan 29, 2025
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