-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
gh-91904: Fix setting envvar PYTHONREGRTEST_UNICODE_GUARD #91905
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
gh-91904: Fix setting envvar PYTHONREGRTEST_UNICODE_GUARD #91905
Conversation
🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit 83046e7e2084773e7c82ef73eb7e928d09fd91d6 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
It always failed on non-UTF-8 locale and prevented running regrtests.
"\N{SMILING FACE WITH SUNGLASSES}", | ||
) | ||
if TESTFN_UNDECODABLE and hasattr(os, 'environb'): | ||
os.environb.setdefault(UNICODE_GUARD_ENV.encode(), TESTFN_UNDECODABLE) |
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.
os.environb is a wrapper to os.environ. You can use os.fsencode()/os.fsdecode() to set the key in os.environ.
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.
Rather os.environ
is a wrapper to os.environb
.
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's fine if you want to use os.environb. It's just that I regret that I added it: surrogateescape of PEP 383 is a better solution to use Unicode (os.environ) on all platforms. It avoids treating Unix differently. os.environb is just here to reduce suprises for Unix users. Internally, os.environ and os.environb are sharing the same Python dict.
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.
And this dict contains bytes.
If use os.environ
, there will be double conversion: encode(decode(value))
. I am not sure that it is lossless in all cases.
83046e7
to
a351699
Compare
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.
LGTM.
🤖 New build scheduled with the buildbot fleet by @serhiy-storchaka for commit f16859b 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10. |
GH-91911 is a backport of this pull request to the 3.10 branch. |
…onGH-91905) It always failed on non-UTF-8 locale and prevented running regrtests. (cherry picked from commit 54d068a) Co-authored-by: Serhiy Storchaka <storchaka@gmail.com>
It always failed on non-UTF-8 locale and prevented running regrtests.
Closes #91904.