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.

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

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

Merged
merged 4 commits into from Jan 15, 2018

Conversation

Projects
None yet
5 participants
@vstinner
Copy link
Member

vstinner commented Oct 30, 2017

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

This comment has been minimized.

Copy link
Contributor

stratakis commented Oct 30, 2017

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

@serhiy-storchaka serhiy-storchaka self-requested a review Nov 29, 2017

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Dec 14, 2017

Ping myself.

@vstinner vstinner force-pushed the vstinner:localeconv branch from b4db317 to 8659161 Dec 18, 2017

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Dec 18, 2017

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

@vstinner vstinner force-pushed the vstinner:localeconv branch 2 times, most recently from dd3a64a to 7a06d07 Jan 10, 2018

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 10, 2018

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 vstinner force-pushed the vstinner:localeconv branch from 7a06d07 to eb1b0fa Jan 10, 2018

@vstinner vstinner requested review from rhettinger and skrah as code owners Jan 10, 2018

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 10, 2018

I completed my change to also fix decimal.Decimal.

@vstinner vstinner force-pushed the vstinner:localeconv branch from eb1b0fa to 7d519dc Jan 10, 2018

@skrah

This comment has been minimized.

Copy link
Member

skrah commented Jan 10, 2018

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

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 10, 2018

@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

This comment has been minimized.

Copy link
Member

skrah commented Jan 10, 2018

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 10, 2018

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.

bpo-31900: Fix localeconv() encoding for LC_NUMERIC
* 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 vstinner force-pushed the vstinner:localeconv branch from c8f1268 to ed16b92 Jan 15, 2018

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 15, 2018

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

@vstinner

This comment has been minimized.

Copy link
Member Author

vstinner commented Jan 15, 2018

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

vstinner added some commits Jan 15, 2018

Don't advice to only call localeconv() at startup
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 vstinner merged commit cb064fc into python:master Jan 15, 2018

4 checks passed

bedevere/issue-number Issue number 31900 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@vstinner vstinner deleted the vstinner:localeconv branch Jan 15, 2018

vstinner added a commit to vstinner/cpython that referenced this pull request Jan 15, 2018

bpo-31900: Fix localeconv() encoding for LC_NUMERIC (python#4174)
* 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)

vstinner added a commit that referenced this pull request Jan 15, 2018

[3.6] bpo-31900: Fix localeconv() encoding for LC_NUMERIC (#4174) (#5192
)

* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.