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

Cast number to Decimal in _get_compact_format #930

Merged

Conversation

DenverCoder1
Copy link
Contributor

@DenverCoder1 DenverCoder1 commented Nov 13, 2022

Fixes #929

Division does not work with decimal.Decimal. This uses // so integer division is performed instead.

abs and round don't work with strings. This converts the number to a Decimal so that _get_compact_format won't raise a ValueError as long as the number supports Decimal conversion. Also has a fix for how rounding modes are treated.

This adds support for decimal.Decimal and str types to the number parameter of format_compact_* functions.

>>> from babel.numbers import *
>>> import decimal
>>> format_compact_decimal(decimal.Decimal("1000"))
"1K"
>>> format_compact_decimal("1000")
"1K"

@codecov
Copy link

codecov bot commented Nov 13, 2022

Codecov Report

Merging #930 (775323a) into master (5fcc253) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #930   +/-   ##
=======================================
  Coverage   91.59%   91.60%           
=======================================
  Files          21       21           
  Lines        4238     4240    +2     
=======================================
+ Hits         3882     3884    +2     
  Misses        356      356           
Impacted Files Coverage Δ
babel/numbers.py 97.94% <100.00%> (+0.01%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@akx
Copy link
Member

akx commented Nov 13, 2022

Initial pre-review thought: I'm pretty hesitant about the possible loss of precision w/ casting to float...

@DenverCoder1
Copy link
Contributor Author

DenverCoder1 commented Nov 13, 2022

As far as I can tell, in compact format, the fractional part never even shows up in any of the formats, so I think even truncating it to an int would be fine (although not necessary).

The method only really needs to support int (and possibly int as a string), but float and Decimal might be nice to make it consistent with the other number formatters.

tests/test_numbers.py Outdated Show resolved Hide resolved
@jun66j5
Copy link
Contributor

jun66j5 commented Nov 15, 2022

I think we could use // operator instead of casting to float.

index 8baf110b8..907ae23f6 100644
--- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -465,7 +465,7 @@ def _get_compact_format(number, compact_format, locale, fraction_digits=0):
                 break
             # otherwise, we need to divide the number by the magnitude but remove zeros
             # equal to the number of 0's in the pattern minus 1
-            number = number / (magnitude / (10 ** (pattern.count("0") - 1)))
+            number = number / (magnitude // (10 ** (pattern.count("0") - 1)))
             # round to the number of fraction digits requested
             number = round(number, fraction_digits)
             # if the remaining number is singular, use the singular format

@jun66j5
Copy link
Contributor

jun66j5 commented Nov 15, 2022

I noticed the format_compact_decimal ignores rounding modes. We should cast to Decimal before applying round() to the number.

index 8baf110b8..6c9ab24b0 100644
--- a/babel/numbers.py
+++ b/babel/numbers.py
@@ -426,7 +426,7 @@ def format_compact_decimal(number, *, format_type="short", locale=LC_NUMERIC, fr
     >>> format_compact_decimal(12345, format_type="long", locale='en_US')
     u'12 thousand'
     >>> format_compact_decimal(12345, format_type="short", locale='en_US', fraction_digits=2)
-    u'12.35K'
+    u'12.34K'
     >>> format_compact_decimal(1234567, format_type="short", locale="ja_JP")
     u'123万'
     >>> format_compact_decimal(2345678, format_type="long", locale="mk")
@@ -454,6 +454,8 @@ def _get_compact_format(number, compact_format, locale, fraction_digits=0):
     The algorithm is described here:
     https://www.unicode.org/reports/tr35/tr35-45/tr35-numbers.html#Compact_Number_Formats.
     """
+    if not isinstance(number, decimal.Decimal):
+        number = decimal.Decimal(str(number))
     format = None
     for magnitude in sorted([int(m) for m in compact_format["other"]], reverse=True):
         if abs(number) >= magnitude:

@DenverCoder1 DenverCoder1 changed the title Cast number to float in _get_compact_format Cast number to Decimal in _get_compact_format Nov 15, 2022
@DenverCoder1
Copy link
Contributor Author

DenverCoder1 commented Nov 15, 2022

Thanks for taking a look! Suggestions applied 👍

@jun66j5
Copy link
Contributor

jun66j5 commented Nov 16, 2022

I found other issue in format_compact_decimal while investigating this issue. Filed in #931.

Copy link
Member

@akx akx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff!

Comment on lines 428 to +429
>>> format_compact_decimal(12345, format_type="short", locale='en_US', fraction_digits=2)
u'12.35K'
u'12.34K'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changing probably depends on the ambient decimal context. But that's okay 👍

@akx akx merged commit 896c2ea into python-babel:master Nov 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for decimal.Decimal and str with format_compact_decimal
3 participants