bpo-34311: Add locale.localize #15275

cedk · 2019-08-14T11:25:08Z

This allow to format Decimal without losing precision by using the new
format syntax and than localize the result.

https://bugs.python.org/issue34311

Misc/NEWS.d/next/Library/2019-08-14-13-19-50.bpo-33731.9esS0d.rst

Lib/test/test_locale.py

Lib/locale.py

Lib/test/test_locale.py

bedevere-bot · 2019-09-11T22:38:50Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

cedk · 2019-09-12T15:13:01Z

I have made the requested changes; please review again

bedevere-bot · 2019-09-12T15:13:05Z

Thanks for making the requested changes!

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

cedk · 2019-09-29T21:35:06Z

ping

cedk · 2019-11-06T15:07:29Z

ping

cedk · 2019-12-03T11:07:23Z

I really would like to see this landed in Python so I could apply a backport to fix https://bugs.tryton.org/issue8574

bharel · 2020-01-09T22:10:34Z

Lib/locale.py

@@ -321,6 +327,11 @@ def delocalize(string):
        string = string.replace(dd, '.')
    return string

+def localize(string, grouping=False, monetary=False):
+    """Parses a string as locale number according to the locale settings."""
+    float(string)


What's the float(string) for?

Ensure that it is a string that represent a valid number.

aeros · 2020-01-12T05:17:00Z

Lib/locale.py

@@ -321,6 +327,11 @@ def delocalize(string):
        string = string.replace(dd, '.')
    return string

+def localize(string, grouping=False, monetary=False):
+    """Parses a string as locale number according to the locale settings."""
+    float(string)


@cedk

Ensure that it is a string that represent a valid number.

Hmm, I'm not sure that I agree with this means of validating the strings. Primarily because it will result in a non-obvious error message for users. For example, localize('') would result in:

ValueError: could not convert string to float: ''

I suspect that this message would make zero sense to users unless they looked at the implementation of the function, which should not be a requirement for diagnosing error messages.

IMO, you should at the very least catch the ValueError and then re-raise it with a more informative error message. For example:

Suggested change

float(string)

try:

float(string)

except ValueError:

raise ValueError(f"{string} is not a valid number")

Edit: It may also be worth considering that there might be some valid floats that you wouldn't consider to be valid for this function, such as float('NAN'), float('INF'), and float('-INF'). For more thorough validation, it could be worthwhile to use regex instead.

Indeed I added this for #15275 (review). But as delocalize does not perform any check and passing a non valid number will not fail, I think it is better to relax the check.

That review link seems to be no longer working. Who was it that originally suggested that change? Particularly if it was from @matrixise (or other core dev), I would highly recommended further discussing the removal of that change with them.

cedk · 2020-01-27T12:29:20Z

ping @matrixise

This allow to format Decimal without losing precision by using the new format syntax and than localize the result.

cedk · 2020-03-31T22:32:38Z

@matrixise could you review the changes you requested, thanks.

aeros · 2020-04-01T22:25:57Z

@cedk If you wouldn't mind, could you avoid force-pushing the PR branch after each commit in the future? It makes it rather difficult to follow the commit history of the PR, before and after the review feedback. We advise against doing so in the devguide:

Your pull request may involve several commits as a result of addressing code review comments. Please keep the commit history in the pull request intact by not squashing, amending, or anything that would require a force push to GitHub. A detailed commit history allows reviewers to view the diff of one commit to another so they can easily verify whether their comments have been addressed. The commits will be squashed when the pull request is merged.

cedk · 2020-04-02T13:48:43Z

@aeros sorry, I would no more do that. Indeed I'm used to mercurial for which such workflow is well supported thanks to evolve.

cedk · 2021-03-27T11:18:00Z

@matrixise could you point me to the changes you are requesting?

matrixise · 2021-04-11T19:26:53Z

@cedk I am sincerely sorry if I was absent, I will look at your PR in the next two weeks.

matrixise · 2021-04-12T10:30:10Z

Hi @cedk ,

As said @aeros , you don't need to force the push, just push with your commits (we can see all the commits and discuss some points), and when we merge the PR, we will use the squash and merge feature. It's pretty useful.

I continue the review of your PR.

Thank you

matrixise · 2021-04-12T10:32:27Z

@cedk just one thing, could you update it with the last master? Thank you so much,

cedk · 2021-04-12T11:52:37Z

@cedk just one thing, could you update it with the last master? Thank you so much,

But there is no conflict.

matrixise · 2021-04-12T12:18:41Z

Thank you so much for your PR, and I am really sorry for this very bid delay between your proposal and the merge. Thank you again for your patience. Take care

the-knights-who-say-ni added the CLA signed label Aug 14, 2019

bedevere-bot added the awaiting review label Aug 14, 2019

epicfaace reviewed Aug 23, 2019

View reviewed changes

Misc/NEWS.d/next/Library/2019-08-14-13-19-50.bpo-33731.9esS0d.rst Outdated Show resolved Hide resolved

Lib/test/test_locale.py Show resolved Hide resolved

Lib/locale.py Show resolved Hide resolved

cedk force-pushed the locale-format branch from 1961096 to 1350ad6 Compare August 23, 2019 18:43

epicfaace suggested changes Aug 23, 2019

View reviewed changes

Lib/locale.py Outdated Show resolved Hide resolved

Lib/locale.py Outdated Show resolved Hide resolved

bedevere-bot added awaiting core review and removed awaiting review labels Aug 23, 2019

cedk force-pushed the locale-format branch 2 times, most recently from 44b7d83 to bcf84aa Compare August 24, 2019 23:03

matrixise self-assigned this Sep 11, 2019

matrixise requested changes Sep 11, 2019

View reviewed changes

Lib/test/test_locale.py Show resolved Hide resolved

bedevere-bot added awaiting changes and removed awaiting core review labels Sep 11, 2019

cedk force-pushed the locale-format branch from bcf84aa to 58344c2 Compare September 12, 2019 15:11

bedevere-bot added awaiting change review and removed awaiting changes labels Sep 12, 2019

bedevere-bot requested a review from matrixise September 12, 2019 15:13

bharel reviewed Jan 9, 2020

View reviewed changes

aeros added the type-feature A feature request or enhancement label Jan 12, 2020

aeros reviewed Jan 12, 2020

View reviewed changes

cedk force-pushed the locale-format branch from 58344c2 to f255f9f Compare January 12, 2020 08:56

cedk force-pushed the locale-format branch from f255f9f to 81785b2 Compare January 23, 2020 13:04

bpo-34311: Add locale.localize

2b5219e

This allow to format Decimal without losing precision by using the new format syntax and than localize the result.

cedk force-pushed the locale-format branch from 81785b2 to 2b5219e Compare March 28, 2020 23:21

cedk and others added 2 commits March 27, 2021 12:06

Merge branch 'master' into locale-format

25aa3a8

Update versionadded

0855860

matrixise approved these changes Apr 12, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Apr 12, 2021

matrixise merged commit e126547 into python:master Apr 12, 2021

bedevere-bot removed the awaiting merge label Apr 12, 2021

cedk deleted the locale-format branch April 12, 2021 13:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-34311: Add locale.localize #15275

bpo-34311: Add locale.localize #15275

cedk commented Aug 14, 2019 •

edited by bedevere-bot

Loading

bedevere-bot commented Sep 11, 2019

cedk commented Sep 12, 2019

bedevere-bot commented Sep 12, 2019

cedk commented Sep 29, 2019

cedk commented Nov 6, 2019

cedk commented Dec 3, 2019

bharel Jan 9, 2020

cedk Jan 10, 2020

aeros Jan 12, 2020 •

edited

Loading

cedk Jan 12, 2020

aeros Jan 12, 2020 •

edited

Loading

cedk commented Jan 27, 2020

cedk commented Mar 31, 2020

aeros commented Apr 1, 2020

cedk commented Apr 2, 2020

cedk commented Mar 27, 2021

matrixise commented Apr 11, 2021

matrixise commented Apr 12, 2021

matrixise commented Apr 12, 2021

cedk commented Apr 12, 2021

matrixise commented Apr 12, 2021

bpo-34311: Add locale.localize #15275

bpo-34311: Add locale.localize #15275

Conversation

cedk commented Aug 14, 2019 • edited by bedevere-bot Loading

bedevere-bot commented Sep 11, 2019

cedk commented Sep 12, 2019

bedevere-bot commented Sep 12, 2019

cedk commented Sep 29, 2019

cedk commented Nov 6, 2019

cedk commented Dec 3, 2019

bharel Jan 9, 2020

Choose a reason for hiding this comment

cedk Jan 10, 2020

Choose a reason for hiding this comment

aeros Jan 12, 2020 • edited Loading

Choose a reason for hiding this comment

cedk Jan 12, 2020

Choose a reason for hiding this comment

aeros Jan 12, 2020 • edited Loading

Choose a reason for hiding this comment

cedk commented Jan 27, 2020

cedk commented Mar 31, 2020

aeros commented Apr 1, 2020

cedk commented Apr 2, 2020

cedk commented Mar 27, 2021

matrixise commented Apr 11, 2021

matrixise commented Apr 12, 2021

matrixise commented Apr 12, 2021

cedk commented Apr 12, 2021

matrixise commented Apr 12, 2021

cedk commented Aug 14, 2019 •

edited by bedevere-bot

Loading

aeros Jan 12, 2020 •

edited

Loading

aeros Jan 12, 2020 •

edited

Loading