-
-
Notifications
You must be signed in to change notification settings - Fork 18.7k
BUG: Fixed Unicode decoding error in Period.strftime
when a locale-specific directive is used
#46405
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
BUG: Fixed Unicode decoding error in Period.strftime
when a locale-specific directive is used
#46405
Changes from 17 commits
d177852
c09b76d
9dff153
2fa0736
28a0921
6b85fb2
409d196
4d09db9
3c1b80c
4c687a8
e01e051
a535249
b4ad6dd
0eafd80
b721238
cebed78
a25f3a9
d175c36
184e480
e5f3062
22b0ae4
bbaa0c1
f6857d6
60c9e69
db0e3cc
06a9567
411ccb7
e5ab44e
fe35aac
0790e72
d6914f4
22c11e7
236a2a0
befa0a0
0105ac9
a1f3773
0fcea6c
7e18375
a1c6d83
78ac13d
3fc0e25
9ad0ee1
406d304
bc1b222
fcd2ce2
b69b074
65c3a1d
e5bca9f
fbfefec
b64548b
4d026cf
cf3be80
8f65bd9
7b414a9
cb43fd5
59b4fe3
4950e06
14f854b
90c31cb
5d467e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,9 +46,13 @@ jobs: | |
- env_file: actions-38.yaml | ||
pattern: "not slow and not network and not single_cpu" | ||
extra_apt: "language-pack-zh-hans" | ||
# Use the utf8 version as the default, it has no bad side-effect. | ||
lang: "zh_CN.utf8" | ||
lc_all: "zh_CN.utf8" | ||
name: "Locale: zh_CN.utf8" | ||
# Also install zh_CN (its encoding is gb2312) but do not activate it. | ||
# It will be temporarily activated during tests with locale.setlocale | ||
extra_loc: "zh_CN" | ||
name: "Locale: zh_CN.utf8 + extra zh_CN (gb2312)" | ||
- env_file: actions-38.yaml | ||
pattern: "not slow and not network and not single_cpu" | ||
pandas_data_manager: "array" | ||
|
@@ -68,6 +72,7 @@ jobs: | |
ENV_FILE: ci/deps/${{ matrix.env_file }} | ||
PATTERN: ${{ matrix.pattern }} | ||
EXTRA_APT: ${{ matrix.extra_apt || '' }} | ||
EXTRA_LOC: ${{ matrix.extra_loc || '' }} | ||
LANG: ${{ matrix.lang || '' }} | ||
LC_ALL: ${{ matrix.lc_all || '' }} | ||
PANDAS_TESTING_MODE: ${{ matrix.pandas_testing_mode || '' }} | ||
|
@@ -138,6 +143,12 @@ jobs: | |
# xsel for clipboard tests | ||
run: sudo apt-get update && sudo apt-get install -y libc6-dev-i386 xsel ${{ env.EXTRA_APT }} | ||
|
||
- name: Generate extra locales | ||
# These extra locales will be available for locale.setlocale() calls in tests | ||
run: | | ||
sudo locale-gen ${{ env.EXTRA_LOC }} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It depends what you mean by "working". Do you refer to the bug #46319 or to something more general ?
Does that answer your question ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was assumed setting the environment variables
would ensure the locale for the test run would not be the default (en_US.UTF-8) for the other locale builds. So IIUC, this is necessary if setting a non-utf8 locale? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are right, this is what I thought too. So this is why I now do not set it through these env variables but I only install it. Maybe the problem is that when the env variables are set, many side effects happen in the build chain, that I do not quite understand. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we're setting the locale this way, can we remove setting the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Honestly I have no idea :) At least for the tests in this PR, yes these env variables are not used. However these env vars are probably related to other locale-related tests, including being able to compile pandas on alternate locale, etc. It seems that in the CI those env variables are used to even add a specific line at the beginning of the init file of pandas ! (see This seems a bit beyond the scope of this PR. However it is maybe worth keeping track of this, in a dedicated ticket. |
||
if: ${{ env.EXTRA_LOC != '' }} | ||
|
||
- uses: conda-incubator/setup-miniconda@v2 | ||
with: | ||
mamba-version: "*" | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -28,6 +28,7 @@ | |||||
timezone, | ||||||
) | ||||||
from decimal import Decimal | ||||||
import locale | ||||||
import operator | ||||||
import os | ||||||
|
||||||
|
@@ -1202,6 +1203,39 @@ def utc_fixture(request): | |||||
utc_fixture2 = utc_fixture | ||||||
|
||||||
|
||||||
@pytest.fixture( | ||||||
params=[ | ||||||
pytest.param(None, id=str(locale.getlocale())), | ||||||
"it_IT.utf8", | ||||||
"zh_CN", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe
Suggested change
What do you think ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be good to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ok, I'll add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will also add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||||||
] | ||||||
) | ||||||
def overridden_locale(request): | ||||||
smarie marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
""" | ||||||
Fixture to temporarily change the locale. | ||||||
|
||||||
If a locale cannot be set (because it is not available on the host) | ||||||
the test is skipped. | ||||||
""" | ||||||
old = locale.setlocale(locale.LC_ALL) | ||||||
target = request.param | ||||||
if target is None: | ||||||
# Current locale - don't change | ||||||
yield old | ||||||
else: | ||||||
try: | ||||||
# Try changing the locale. | ||||||
locale.setlocale(locale.LC_ALL, target) | ||||||
except locale.Error as e: | ||||||
# Not available on this host. Skip test. | ||||||
pytest.skip(f"Skipping as locale cannot be set. {type(e).__name__}: {e}") | ||||||
else: | ||||||
# Run test with the temporary local | ||||||
yield target | ||||||
# Set back to normal | ||||||
locale.setlocale(locale.LC_ALL, old) | ||||||
|
||||||
|
||||||
# ---------------------------------------------------------------- | ||||||
# Dtypes | ||||||
# ---------------------------------------------------------------- | ||||||
|
Uh oh!
There was an error while loading. Please reload this page.