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

Allow full control on Decimal behaviour #410

Merged
merged 3 commits into from
Jul 8, 2016
Merged

Allow full control on Decimal behaviour #410

merged 3 commits into from
Jul 8, 2016

Conversation

etanol
Copy link
Contributor

@etanol etanol commented May 29, 2016

It turns out that the solution to allow the user to change the rounding mode was way simpler than expected at the beginning.

According to UTS #35 sections 3.3 and 3.7, the user is allowed to change rounding mode. But the default should be half even mode. This is already the default Python rounding mode in all decimal implementations, so the solution was to stop indicating the rounding mode on Decimal operations.

Some documentation has been added to show how to properly change the rounding mode and other Context parameters.

Therefore, this should fix #90

Allow the rounding mode to be controlled from the currently active decimal
context.  This gives the caller the ability to control rounding mode, precision,
exponent range and all attributes that affect decimal operations.

Fixes #90
Instead of selecting individual symbols, expose only the chosen decimal module
through babel._compat.  This forces Babel to always use "decimal." prefix.
Howver, it will simplify the code for users that need to manipulate the decimal
context.
Explain how the user can control the behaviour of number rounding when
formatting numeric values or currencies.
ROUND_HALF_EVEN = _RHE
if sys.version_info[:2] >= (3, 3):
import decimal
else:

Choose a reason for hiding this comment

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

This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section default.

The issue can be fixed by applying the following patch:

--- a/babel/_compat.py
+++ b/babel/_compat.py
@@ -67,4 +67,4 @@
     try:
         import cdecimal as decimal
     except ImportError:
-        import decimal
+        pass

Choose a reason for hiding this comment

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

This file contains unused source code.

PyUnusedCodeBear, severity NORMAL, section default.

The issue can be fixed by applying the following patch:

--- a/babel/_compat.py
+++ b/babel/_compat.py
@@ -67,4 +67,4 @@
     try:
         import cdecimal as decimal
     except ImportError:
-        import decimal
+        pass

Copy link
Contributor Author

@etanol etanol May 29, 2016

Choose a reason for hiding this comment

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

This seems odd, according to Codecov, babel/_compat.py is 100% covered.

@codecov-io
Copy link

codecov-io commented May 29, 2016

Current coverage is 89.51%

Merging #410 into master will decrease coverage by 0.64%

  1. 2 files (not in diff) in babel/localtime were modified. more
    • Misses +21
    • Hits -21
  2. 1 files (not in diff) in babel were modified. more
    • Misses +4
    • Hits -4
@@             master       #410   diff @@
==========================================
  Files            24         24          
  Lines          3950       3946     -4   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
- Hits           3561       3532    -29   
- Misses          389        414    +25   
  Partials          0          0          

Powered by Codecov. Last updated by 8ebdac3...f12e975

@akx
Copy link
Member

akx commented May 29, 2016

ack 6238f66 8341064 f12e975

@akx
Copy link
Member

akx commented May 29, 2016

LGTM, but if someone else (@jtwang?) wants to give this an once-over, that'd be grand. :)

@@ -86,6 +86,58 @@ the specification. The following table is just a relatively brief overview.
+----------+-----------------------------------------------------------------+


Rounding Modes
Copy link
Contributor

Choose a reason for hiding this comment

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

+++++++

Thanks for the great docs!

Could you add a doctest or a note to the docstring for babel.numbers.format_decimal? Same for format_currency - especially around the interaction with custom currency formats and currency_digits. (omg so complicated)

@jtwang
Copy link
Contributor

jtwang commented Jun 1, 2016

Thank you so much for taking this on!

@sublee - does this let you do what you want?

@sublee
Copy link
Contributor

sublee commented Jun 1, 2016

@jtwang No it doesn't. What I need is the precision setting for format_number and the similar functions. Not rounding option.

Precision to show usually depend on business logic, not specific locale. The code for determining precision should be static against dynamic locales. For example:

>>> print(format_number(123456789.123456789, prec=5, locale=random.choice(locales)))
123.456.789,12346

For now, we can't separate precision from locales. Because format_number detects precision from a format option but the format string comes from locales.

@etanol
Copy link
Contributor Author

etanol commented Jun 1, 2016

The rounding performed by Babel is only triggered when formatting a numeric or monetary amount requires digit truncation. To control how many decimals you wan to display, you can always use the format argument of the babel.numbers.format_decimal function, independently of the locale. For example:

>>> from babel.numbers import format_decimal
>>> format_decimal(1234.1, format='#,##0.000', locale='en_US')
u'1,234.100'
>>> format_decimal(1234.1, format='#,##0.00##', locale='en_US')
u'1,234.10'
>>> format_decimal(1234.123456, format='#,##0.00##', locale='en_US')
u'1,234.1235'

Also note that you can pass instances of Decimal (properly imported, once this PR is merged) to the number formatting functions. And, if you are serious about decimal number precision, such as in finances, you should be using Decimal everywhere in your business logic.

@sublee
Copy link
Contributor

sublee commented Jun 1, 2016

@etanol Custom format options work well with several locales but not whole of the locales.

The #,##0.00## format, you presented, is proper for en_US or ru_RU because they have the same length of comma groups. But numbers in hi_IN has a different comma rule. Commas should be inserted every 2 digits except the first group. Only the first group has 3 digits especially. In other words, the default number format of hi_IN is #,##,##0.###.

So, if I want to set a precision by the format option, I should know the comma rule of the locale I will use:

>>> format_number(123456789.123456789, locale='hi_IN')
'12,34,56,789.123'
>>> # The format is yours.  The precision specified exactly but commas are wrong.
>>> format_number(123456789.123456789, format='#,##0.00##', locale='hi_IN')
'123,456,789.1235'
>>> # All is okay but the format depends on the hi_IN locale.
>>> format_number(123456789.123456789, format='#,##,##0.00##', locale='hi_IN')
'12,34,56,789.1235'

As I said, it depends on locales instead of business logic.

@etanol
Copy link
Contributor Author

etanol commented Jun 3, 2016

@sublee Right, I see what you mean. I have a couple of ideas to refactor the babel.numbers module and maybe expose some tuning over the number formatting by skipping over string patterns. But due to lack of spare time, don't expect any change (at least from my part) until the end of this year.

@sublee
Copy link
Contributor

sublee commented Jun 7, 2016

@etanol Sounds great! I'll wait your work.

@akx
Copy link
Member

akx commented Jul 8, 2016

So yeah, even if this doesn't exactly fix #90, I think it's a step in a nice direction.

@jtwang, @sublee, what do you think, shall we merge?

@jtwang
Copy link
Contributor

jtwang commented Jul 8, 2016

@akx - sure, I didn't spot anything worrisome. :)

@akx akx merged commit 9a3ce80 into python-babel:master Jul 8, 2016
@kdeldycke kdeldycke mentioned this pull request Apr 20, 2017
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.

Bypass number rounding
6 participants