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 decimal for LC_NUMERIC != LC_CTYPE #5191

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
4 participants
@vstinner
Copy link
Member

vstinner commented Jan 15, 2018

Fix decimal.Decimal formatter when the LC_NUMERIC locale encoding is
different than the LC_CTYPE encoding: reuse
_Py_GetLocaleconvNumeric() to decode localeconv() correctly, set
tempoarily setlocale() if needed.

https://bugs.python.org/issue31900

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 15, 2018

@skrah: So, what do you think?

@skrah
Copy link
Member

skrah left a comment

The patched version introduces errors in deccheck.py. The formatting code is tricky, which is why I wasn't too enthusiastic about the patch to begin with.

I'd prefer to do nothing here.

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 15, 2018

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

bpo-31900: Fix decimal for LC_NUMERIC != LC_CTYPE
Fix decimal.Decimal formatter when the LC_NUMERIC locale encoding is
different than the LC_CTYPE encoding: reuse
_Py_GetLocaleconvNumeric() to decode localeconv() correctly, set
tempoarily setlocale() if needed.

Add mpd_spec_t.locale field.

@vstinner vstinner force-pushed the vstinner:decimal_dot branch from 65c6399 to 3af5cfa Jan 23, 2018

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 23, 2018

The patched version introduces errors in deccheck.py.

Oh. My type == 'n' test doesn't work, libmpdec replaces 'n' with 'g'. I don't know how I missed this.

I modified my PR to add a new mpd_spec_t.locale field.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 23, 2018

I have made the requested changes; please review again.

@bedevere-bot

This comment has been minimized.

Copy link

bedevere-bot commented Jan 23, 2018

Thanks for making the requested changes!

@skrah: please review the changes made to this pull request.

@vstinner

This comment has been minimized.

Copy link
Member

vstinner commented Jan 30, 2018

@skrah: I fixed my obvious mistake. Would you mind to look again to my change? Is it now ok to merge it?

@skrah

This comment has been minimized.

Copy link
Member

skrah commented Jan 30, 2018

@vstinner I'll look at it when I have time. I still think this is nice-to-have, but very low priority.

@vstinner vstinner closed this Feb 1, 2018

@vstinner vstinner deleted the vstinner:decimal_dot branch May 29, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment