Skip to content

Fix: Handle None value for locale in format_currency#1158

Closed
bestpantusen wants to merge 2 commits into
python-babel:masterfrom
bestpantusen:fix-issue-1156
Closed

Fix: Handle None value for locale in format_currency#1158
bestpantusen wants to merge 2 commits into
python-babel:masterfrom
bestpantusen:fix-issue-1156

Conversation

@bestpantusen

Copy link
Copy Markdown

Summary

This PR fixes the issue where passing None as the locale to babel.numbers.format_currency caused a TypeError. The function now defaults to the system's default locale when locale=None.

Related Issue

Closes #1156

Changes Made

  • Updated format_currency in babel/numbers.py to default to default_locale() when locale=None.
  • Added tests to tests/test_numbers.py:
    • test_format_currency_with_none_locale

Comment thread babel/numbers.py
decimal_quantization=decimal_quantization, group_separator=group_separator,
numbering_system=numbering_system)
if locale is None:
locale = default_locale()

Copy link
Copy Markdown

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().

@injust

injust commented Dec 8, 2024

Copy link
Copy Markdown

Keep in mind that default_locale() can return None. Not sure if there's a good fallback in that case.

Comment thread babel/numbers.py
decimal_quantization=decimal_quantization, group_separator=group_separator,
numbering_system=numbering_system)
if locale is None:
locale = default_locale()

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.

Suggested change
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 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.

I agree with @tomasr8 – this should be fixed for all functions that claim to accept locale=None.

@akx

akx commented Jan 8, 2025

Copy link
Copy Markdown
Member

Ah, this is actually even worse on Windows, where default_locale will happily return None in most cases (since none of the POSIX-y environment variables are defined)...

@tomasr8

tomasr8 commented Jan 8, 2025

Copy link
Copy Markdown
Member

Perhaps instead of allowing None in these functions, we could do the opposite and simply remove None from the type annotations to align them with the actual behaviour?

@akx

akx commented Jan 8, 2025

Copy link
Copy Markdown
Member

Perhaps instead of allowing None in these functions, we could do the opposite and simply remove None from the type annotations to align them with the actual behaviour?

That could break current user code ("just format this whatever in whatever is the current default locale" – rare, but possible).

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.

locale parameter cannot be None

4 participants