-
Notifications
You must be signed in to change notification settings - Fork 460
Fix: Handle None value for locale in format_currency #1158
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
Conversation
| decimal_quantization=decimal_quantization, group_separator=group_separator, | ||
| numbering_system=numbering_system) | ||
| if locale is None: | ||
| locale = default_locale() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It bugs me that default_locale() determines the locale as a string, parses it to a Locale, and converts it to its string identifier. Then immediately after, it's parsed back to a Locale.
But I can't think of a good way around this, other than a breaking change in default_locale().
|
Keep in mind that |
| decimal_quantization=decimal_quantization, group_separator=group_separator, | ||
| numbering_system=numbering_system) | ||
| if locale is None: | ||
| locale = default_locale() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| locale = default_locale() | |
| locale = LC_NUMERIC |
Though I'm not sure if it makes sense to just fix this one function. A lot of the other format_* functions cannot deal with explicitly passed None as the locale either:
from babel.numbers import format_compact_currency
format_compact_currency('12', 'EUR', locale=None)I think if we're gonna fix this, we should fix all of them at the same time, otherwise we're gonna have functions with the same type annotation behaving differently which could lead to confusion.
akx
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @tomasr8 – this should be fixed for all functions that claim to accept locale=None.
|
Ah, this is actually even worse on Windows, where |
|
Perhaps instead of allowing |
That could break current user code ("just format this whatever in whatever is the current default locale" – rare, but possible). |
Summary
This PR fixes the issue where passing
Noneas thelocaletobabel.numbers.format_currencycaused a TypeError. The function now defaults to the system's default locale whenlocale=None.Related Issue
Closes #1156
Changes Made
format_currencyinbabel/numbers.pyto default todefault_locale()whenlocale=None.tests/test_numbers.py:test_format_currency_with_none_locale