Code

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#15789 closed Bug (fixed)

floatformat filter works incorrectly when decimal precision is set low

Reported by: akaihola Owned by: igalarzab
Component: Template system Version: 1.3
Severity: Normal Keywords:
Cc: eire1130@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

When floatformat receives a decimal places argument which is equal or larger than the current decimal precision, the decimal part isn't truncated. An example:

>>> from django.template.defaultfilters import floatformat
>>> import decimal
>>> decimal.getcontext().prec = 2
>>> floatformat(1.23456789123, 1)
u'1.2'
>>> floatformat(1.23456789123, 2)
u'1.23456789123'
>>> floatformat(1.23456789123, 3)
u'1.23456789123'

Attachments (4)

patch.diff (2.1 KB) - added by igalarzab 3 years ago.
Patch
patch.2.diff (2.8 KB) - added by igalarzab 3 years ago.
patch2.diff (2.8 KB) - added by igalarzab 3 years ago.
Better patch
patch3.diff (3.0 KB) - added by igalarzab 3 years ago.
Patch with more test functions

Download all attachments as: .zip

Change History (20)

comment:1 Changed 3 years ago by akaihola

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

What happens inside floatformat when evaluating decimal.getcontext().prec = 2 ; floatformat(1.23456789123, 2) is equivalent to this:

>>> import decimal
>>> decimal.getcontext().prec = 2
>>> d = decimal.Decimal('1.23456789123')
>>> exp = decimal.Decimal('0.01')
>>> d.quantize(exp, decimal.ROUND_HALF_UP)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python2.6/decimal.py", line 2358, in quantize
    'quantize result has too many digits for current context')
  File "/usr/lib/python2.6/decimal.py", line 3778, in _raise_error
    raise error(explanation)
decimal.InvalidOperation: quantize result has too many digits for current context

At this point, floatformat eats the exception and just returns the original value without truncating the decimal part.

comment:2 Changed 3 years ago by mir

  • Easy pickings set
  • Needs tests set
  • Triage Stage changed from Unreviewed to Accepted

Changed 3 years ago by igalarzab

Patch

comment:3 Changed 3 years ago by igalarzab

  • Has patch set
  • Needs tests unset
  • Owner changed from nobody to igalarzab
  • Status changed from new to assigned

Hi,

I found some more problems. For example, with a precission of two digits, this regression test fails:

self.assertEqual(floatformat(13.1031, -3), u'13.103')

I think this patch solves these problems.

Last edited 3 years ago by igalarzab (previous) (diff)

comment:4 Changed 3 years ago by ptone

  • UI/UX unset

The current patch no longer applies cleanly to trunk. I tried updating the patch, believe I updated it accurately, but still had test failures

    FAIL: test_l10n_disabled (regressiontests.i18n.tests.FormattingTests)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
    File "/Users/preston/Projects/code/forks/django/tests/regressiontests/i18n/tests.py", line 226, in test_l10n_disabled
        self.assertEqual(u'66666.67', Template('{{ n|floatformat:2 }}').render(self.ctxt))
    AssertionError: u'66666.67' != u'66666.666'

    ======================================================================
    FAIL: test_l10n_enabled (regressiontests.i18n.tests.FormattingTests)
    ----------------------------------------------------------------------
    Traceback (most recent call last):
    File "/Users/preston/Projects/code/forks/django/tests/regressiontests/i18n/tests.py", line 320, in test_l10n_enabled
        self.assertEqual(u'66666,67', Template('{{ n|floatformat:2 }}').render(self.ctxt))
    AssertionError: u'66666,67' != u'66666.666'

Sorry - kicking the can....

comment:5 Changed 3 years ago by igalarzab

I applied my patch to the trunk, and I've sent a pull request on github: https://github.com/django/django/pull/44

comment:6 Changed 3 years ago by ptone

  • Patch needs improvement set

The current pull applies cleanly - but still fails tests and doing some quick experiments appears to fail to perform - perhaps it is in the absence of a decimal context precision set explicitly

>>> from django.template.defaultfilters import floatformat
>>> import decimal
>>> x = decimal.Decimal('5555.555')
>>> y = 5555.555
>>> floatformat(x,2)
u'5555.555'
>>> floatformat(y,2)
u'5555.555'
>>> 

Changed 3 years ago by igalarzab

comment:7 Changed 3 years ago by igalarzab

Yes, you are right!

I resubmit the patch (patch2.diff), what do you think? :)

Last edited 3 years ago by igalarzab (previous) (diff)

Changed 3 years ago by igalarzab

Better patch

comment:8 Changed 3 years ago by ptone

  • Needs tests set

It might be nice to add a test for the decimal type since you already have decimal imported:

Not stopper, but since you seem to be responsive currently for this ticket, would you mind adding?

self.assertEqual(floatformat(decimal.Decimal('5555.555', 2), u'5555.56')

Otherwise this is looking good.

comment:9 Changed 3 years ago by igalarzab

  • Needs tests unset
  • Patch needs improvement unset

Of course!

I added more test funcionts in the last patch (about the decimal module).

Thank you very much for your review :)

Changed 3 years ago by igalarzab

Patch with more test functions

comment:10 Changed 3 years ago by ptone

  • milestone set to 1.4
  • Triage Stage changed from Accepted to Ready for checkin

Latest patch look good

comment:11 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

comment:12 Changed 2 years ago by aaugustin

I'm going to improve a little bit the patch before committing it. In particular decimal.getcontext().prec should be restored at the end of the test.

comment:13 Changed 2 years ago by aaugustin

  • Resolution set to fixed
  • Status changed from assigned to closed

In [17297]:

Fixed #15789 -- Set the decimal precisio to avoid an exception in the floatformat filter, and added a few more tests. Thanks akaihola for the report, igalarzab for the patch and ptone for the review.

comment:14 Changed 2 years ago by eire1130

  • Cc eire1130@… added
  • Version changed from master to 1.3

Can a commiter send this patch to 1.3? It's in 1.4, but it would be helpful to have this in 1.3

comment:15 Changed 2 years ago by charettes

Only security fixes are backported to the 1.3.X branch.

See this page for more information concerning backport policy.

Last edited 2 years ago by charettes (previous) (diff)

comment:16 Changed 2 years ago by akaariai

A quick correction: actually, only security fixes are applied to 1.3.X. I got that wrong, too: see #18135.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.