-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
Internationalise the decimal separator in intcomma()
#53
Internationalise the decimal separator in intcomma()
#53
Conversation
8c89d2a
to
496b5a8
Compare
Codecov Report
@@ Coverage Diff @@
## main #53 +/- ##
==========================================
+ Coverage 99.28% 99.29% +0.01%
==========================================
Files 9 9
Lines 696 710 +14
==========================================
+ Hits 691 705 +14
Misses 5 5
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
try: | ||
humanize.i18n.activate("de_DE") | ||
assert humanize.intcomma(number) == "10.000.000" | ||
assert humanize.intcomma(1_234_567.8901) == "1.234.567,8901" | ||
assert humanize.intcomma(1_234_567.89) == "1.234.567,89" | ||
assert humanize.intcomma("1234567,89") == "1.234.567,89" | ||
assert humanize.intcomma("1.234.567,89") == "1.234.567,89" | ||
assert humanize.intcomma("1.234.567,8") == "1.234.567,8" | ||
|
||
humanize.i18n.activate("fr_FR") | ||
assert humanize.intcomma(number) == "10 000 000" |
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.
humanize.i18n.activate("EN")
assert humanize.intcomma(1_234_567.8901) == "1,234,567.8901"
It would be good to add a test for a non-German locale to show that the original behavior is preserved. @hugovk, are the doctests verified by the CI?
Also, it should be documented that if the input is a string, it must already match the locale. i.e. This function is not to be used to convert a string from one locale to another.
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 would be good to add a test for a non-German locale
There are already tests for the default locale. Or do you mean a non-German, non-default locale?
Also, it should be documented that if the input is a string, it must already match the locale.
This was already the case with the thousands separator btw. Where would be a good place to document this?
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.
There are already tests for the default locale. Or do you mean a non-German, non-default locale?
This is exactly it. I didn't realize localization tests were separate from default locale tests. Thanks!
Where would be a good place to document this?
In the intcomma docstring would be the best place. It may already be the case, but it is not documented as such.
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.
I'm not sure how best to write this, given that it doesn't really even mention that strings are allowed, except in two examples and partially in the arguments.
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 would be good to add a test for a non-German locale to show that the original behavior is preserved. @hugovk, are the doctests verified by the CI?
Yes, doctests are run by pytest and pytest is run on the CI.
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.
Implementation closely follows existing implementation for thousands separation localization. Tests are sufficient. LGTM!
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.
Thank you for this! Just a few little nits :)
496b5a8
to
862748a
Compare
Done. |
Thank you! |
intcomma()
See discussion in #48.
Changes proposed in this pull request: