Repository: python-babel/babel

Issue:
Babel 2.17 regression: TypeError on Windows for `locale=''`
Reproducer:

```python
import datetime

import babel
from babel.dates import format_date

date = datetime.date(2016, 2, 7)
try:
    print(format_date(date, locale=''))
except (ValueError, babel.UnknownLocaleError):
    print('Invalid Babel locale: ...')
```

Output:

```ps1con
PS> python bug.py
Traceback (most recent call last):
  File "...\bug.py", line 8, in <module>
    print(format_date(date, locale=''))
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\Lib\site-packages\babel\dates.py", line 705, in format_date
    locale = Locale.parse(locale or LC_TIME)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "...\Lib\site-packages\babel\core.py", line 350, in parse
    raise TypeError(msg)
TypeError: ("Empty locale identifier value: None\n\nIf you didn't explicitly pass an empty value to a Babel function, this could be caused by there being no suitable locale environment variables for the API you tried to use.",)
```

This is minimised from Sphinx's test suite, where we've newly been seeing failures only on Windows. After much attempted debugging, I noticed Babel had a recent release. With 2.16, the error message is printed, but in 2.17 a TypeError is raised.

I think this is a regression as `locale=''` is a valid type (`str`), but an invalid value. The new code uses `locale or LC_TIME`, which is `None` on Windows:

https://github.com/python-babel/babel/blob/b50a1d2186c20f3359f7e10853d2b2225a46ed40/babel/dates.py#L705

A

Discussion:
@tomasr8: Hi! This was changed in https://github.com/python-babel/babel/pull/1164. `Locale.parse` now explicitly rejects `None` with a `TypeError` though I agree that this should not happen when you explicitly pass a string to one of the `format_` functions. What do you think @akx ?@AA-Turner: Right, the problem is that the change now uses the expression `locale or LC_TIME`, which short-circuits to `LC_TIME`, which is `None` on Windows (but presumably not `None` on Unix and macOS). @tomasr8: Indeed, this is still expected to fail, but it should IMO continue to fail with `ValueError`, not `TypeError` @akx: So the change was originally made (among other reasons) so passing an empty string at the top-most API level for a locale wouldn't give you a cryptic `ValueError: expected only letters, got ''` error:

```
$ env LC_TIME=vööh uvx --from=babel==2.16.0 python reproducer.py
[...]
ValueError: expected only letters, got ''

$ env LC_TIME=vööh uvx --from=babel==2.17.0 python reproducer.py
[...]
babel.core.UnknownLocaleError: unknown locale 'vööh'
```

Even more so, we now have a better error message (though now I noticed the error message is a 1-tuple of a string, because I'm a silly goose) when the value is not ambiently available either:

```
$ env LC_TIME= LANG= uvx --from=babel==2.16.0 python reproducer.py
[...]
ValueError: expected only letters, got ''

$ env LC_TIME= LANG= uvx --from=babel==2.17.0 python reproducer.py
TypeError: ("Empty locale identifier value: None...
```

I'm not 100% sure about making the latter exception a ValueError, because the value that gets eventually passed down to `Locale.parse()` is in this case indeed the wrong type – and we have a special case to keep raising a ValueError (for legacy compat, which obviously didn't exactly do the thing here...) if the value passed in to `Locale.parse()` is a string:

```
$ env LC_TIME= LANG= uvx --from=babel==2.17.0 python
>>> from babel import Locale
>>> Locale.parse("")
ValueError: ("Empty locale identifier value: ''...
>>> Locale.parse(None)
TypeError: ("Empty locale identifier value: None...
```

Trouble is of course we don't currently know whether our end-user is explicitly passing in a silly value, or if the silly value comes from the defaulting-from-environment behavior.

One option could be to make the higher-level functions do `Locale.parse(locale or LC_TIME or _UNSET_LOCALE)` (a sentinel value) (c.f. the current `Locale.parse(locale or LC_TIME)`) that we could explicitly detect and `ValueError` in `Locale.parse`.

🤔 

Btw, @AA-Turner: does Sphinx's test suite explicitly/purposely unset locale-related environment variables, or should we also be wondering why no time locale gets detected on Windows?@AA-Turner: > does Sphinx's test suite explicitly/purposely unset locale-related environment variables, or should we also be wondering why no time locale gets detected on Windows?

Not that I'm aware of. I ran the reproducer locally (Windows 11) and got identical output, so I imagine it is not specific to the test suite.

A@AA-Turner: Could you introduce a helper function?:

```python
if locale:
    return locale
if LC_TIME:
    return LC_TIME
return '' if locale == '' else LC_TIME
```@tomasr8: > One option could be to make the higher-level functions do Locale.parse(locale or LC_TIME or _UNSET_LOCALE)

But then if the passed `locale` happens to be `None` and `LC_TIME` is unset we'd raise a ValueError instead of TypeError, right? I don't think we want that.

Maybe explicitly checking for an empty string as Adam is suggesting would work?@JoseG2003-g: Hi, could me and my team tackle this issue ?@tomasr8: Sure, but first we need to figure out what the best course of action is here. @akx what do you think we should do?@akx: Sorry for the delay...

Right, I see Sphinx [does](https://github.com/sphinx-doc/sphinx/blob/2fa51bb49351cff4e3c67adafbe51ecfd0fdb43b/sphinx/util/i18n.py#L231-L241)

```python
    try:
        return formatter(date, format, locale=locale)
    except (ValueError, babel.core.UnknownLocaleError):
        # fallback to English
        logger.warning(__('Invalid Babel locale: %r.'), ...)
        return formatter(date, format, locale='en')
```

so where the test suite explicitly passes in `""`, we used to interpret that in a way that has Sphinx's machinery fall back to English. With 2.17, the `or` expression defers that to `None` from the locale, which now raises a different exception – IOW, this means Sphinx users on Windows could be in production trouble too if `sphinx.util.i18n.format_date` is ever called with a falsy `language`.

Looking at the callers for `format_date()` in Sphinx, it seems they always pass in `self.config.language`, which is guarded to not be `None` via https://github.com/sphinx-doc/sphinx/pull/10481. (It's not guarded against being an empty string there – maybe it should be?)

It looks like the test case that explicitly passes in an empty string to `format_date()` was added in https://github.com/sphinx-doc/sphinx/commit/b780d2207a2bf1a63b98fa0583d3903638ead026.

I think the best course of action would actually be to adjust _Sphinx_ a bit:

* guard against all falsy `language`s while parsing configuration, and default them to `"en"`
* remove the test case that explicitly passes in `""` as the language (as that would never happen for Sphinx users in real life with the above change).

WDYT, @AA-Turner? (EDIT: I opened a PR to do this, just in case. https://github.com/sphinx-doc/sphinx/pull/13408)
@AA-Turner: I've merged @akx's PR (thank you!). Do you want to keep this issue open for investigating the Windows locale detection failure?

A@akx: I think we can close this – it would be nice to have a default locale on Windows too (we currently don't, really, unless the user has set POSIX style `LANG`/`LC_*` environment variables), but that's out of scope for this issue.

Glad we sorted the actual issue out!

Solution patch:
diff --git a/babel/core.py b/babel/core.py
index 5762bbe..7386fb3 100644
--- a/babel/core.py
+++ b/babel/core.py
@@ -343,7 +343,7 @@ class Locale:
                 f"Empty locale identifier value: {identifier!r}\n\n"
                 f"If you didn't explicitly pass an empty value to a Babel function, "
                 f"this could be caused by there being no suitable locale environment "
-                f"variables for the API you tried to use.",
+                f"variables for the API you tried to use."
             )
             if isinstance(identifier, str):
                 raise ValueError(msg)  # `parse_locale` would raise a ValueError, so let's do that here


Test patch:
diff --git a/tests/test_core.py b/tests/test_core.py
index aaf95a1..8c215de 100644
--- a/tests/test_core.py
+++ b/tests/test_core.py
@@ -395,8 +395,9 @@ def test_language_alt_official_not_used():
 
 
 def test_locale_parse_empty():
-    with pytest.raises(ValueError, match="Empty"):
+    with pytest.raises(ValueError, match="Empty") as ei:
         Locale.parse("")
+    assert isinstance(ei.value.args[0], str)
     with pytest.raises(TypeError, match="Empty"):
         Locale.parse(None)
     with pytest.raises(TypeError, match="Empty"):