Skip to content
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.

Sign up for GitHub

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

Jump to bottom

bpo-31900: Fix localeconv() encoding for LC_NUMERIC #4174

Merged
merged 4 commits into from
Jan 15, 2018
Merged

bpo-31900: Fix localeconv() encoding for LC_NUMERIC #4174

merged 4 commits into from
Jan 15, 2018

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Oct 30, 2017 •
edited by bedevere-bot
Loading

Decode localeconv() numeric fields like thousands_sep from the
LC_NUMERIC encoding, rather than decoding from the LC_CTYPE encoding.

https://bugs.python.org/issue31900

@stratakis
Copy link
Contributor

Tested the PR on a system with glibc 2.26.90 where the test was failing, and it successfully passed.

@vstinner
Copy link
Member Author

Ping myself.

@vstinner
Copy link
Member Author

I rebased my changed, fixed a bug in error handler (restore the LC_CTYPE if RESULT fails), and added documentation.

@vstinner
Copy link
Member Author

I completed my change to also fix str.format():

# Bug in Python 3.6
vstinner@apu$ env -i python3 -c 'import locale; locale.setlocale(locale.LC_ALL, "fr_FR"); locale.setlocale(locale.LC_NUMERIC, "es_MX.utf8"); print(ascii(f"{1000:n}"))'
'1\xe2\x80\x89000'

# Fixed in master
vstinner@apu$ env -i ./python -c 'import locale; locale.setlocale(locale.LC_ALL, "fr_FR"); locale.setlocale(locale.LC_NUMERIC, "es_MX.utf8"); print(ascii(f"{1000:n}"))'
'1\u2009000'

@vstinner
Copy link
Member Author

I completed my change to also fix decimal.Decimal.

@skrah
Copy link
Contributor

skrah commented Jan 10, 2018

@vstinner Cool, but could you make the decimal part a separate PR?

@vstinner
Copy link
Member Author

@vstinner Cool, but could you make the decimal part a separate PR?

Would you mind to elaborate? Why should it be fixed in a different PR?

@skrah
Copy link
Contributor

skrah commented Jan 10, 2018 via email

@vstinner
Copy link
Member Author

Because we both thought that this issue was not very important a couple of years ago

Well, something changed in the meanwhile, the glibc: https://bugs.python.org/issue31900#msg305227

Anyway, I reverted changes on the decimal module.

@vstinner
bpo-31900: Fix localeconv() encoding for LC_NUMERIC
ed16b92 
* Add _Py_GetLocaleconvNumeric() function: decode decimal_point and
  thousands_sep fields of localeconv() from the LC_NUMERIC encoding,
  rather than decoding from the LC_CTYPE encoding.
* Modify locale.localeconv() and "n" formatter of str.format() (for
  int, float and complex to use _Py_GetLocaleconvNumeric()
  internally.
@vstinner
Copy link
Member Author

I rebased this change on top my merged commit 7ed7aea (bpo-29240). I also fixed error handling.

@vstinner
Copy link
Member Author

Ah, moreover I avoided the setlocale() if LC_NUMERIC = LC_CTYPE.

@vstinner
Fix clang warnings on FreeBSD
07e2852
@vstinner
Don't advice to only call localeconv() at startup
ab9cac3 
The setlocale() is only done in a rare use case (LC_NUMERIC and
LC_CTYPE have a different encoding), it's safe to call localeconv()
in all other cases.
@vstinner
Don't declare 'loc' variable twice
5a400c4
@vstinner vstinner merged commit cb064fc into python:master Jan 15, 2018
@vstinner vstinner deleted the localeconv branch January 15, 2018 14:58
vstinner added a commit that referenced this pull request Jan 15, 2018
@vstinner
[3.6] bpo-31900: Fix localeconv() encoding for LC_NUMERIC (#4174) (#5192
5f959c4 
)

* Add _Py_GetLocaleconvNumeric() function: decode decimal_point and
  thousands_sep fields of localeconv() from the LC_NUMERIC encoding,
  rather than decoding from the LC_CTYPE encoding.
* Modify locale.localeconv() and "n" formatter of str.format() (for
  int, float and complex to use _Py_GetLocaleconvNumeric()
  internally.

(cherry picked from commit cb064fc)
@serhiy-storchaka serhiy-storchaka mentioned this pull request Oct 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

@rhettinger rhettinger Awaiting requested review from rhettinger

@skrah skrah Awaiting requested review from skrah

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@vstinner @stratakis @skrah @the-knights-who-say-ni @bedevere-bot