Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#15789 closed Bug (fixed)

floatformat filter works incorrectly when decimal precision is set low

Reported by: Antti Kaihola Owned by: Jose Ignacio Galarza
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 Jose Ignacio Galarza 14 years ago.
Patch
patch.2.diff (2.8 KB ) - added by Jose Ignacio Galarza 13 years ago.
patch2.diff (2.8 KB ) - added by Jose Ignacio Galarza 13 years ago.
Better patch
patch3.diff (3.0 KB ) - added by Jose Ignacio Galarza 13 years ago.
Patch with more test functions

Download all attachments as: .zip

Change History (20)

comment:1 by Antti Kaihola, 14 years ago

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 by Michael Radziej, 14 years ago

Easy pickings: set
Needs tests: set
Triage Stage: UnreviewedAccepted

by Jose Ignacio Galarza, 14 years ago

Attachment: patch.diff added

Patch

comment:3 by Jose Ignacio Galarza, 14 years ago

Has patch: set
Needs tests: unset
Owner: changed from nobody to Jose Ignacio Galarza
Status: newassigned

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 14 years ago by Jose Ignacio Galarza (previous) (diff)

comment:4 by Preston Holmes, 13 years ago

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 by Jose Ignacio Galarza, 13 years ago

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 by Preston Holmes, 13 years ago

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'
>>> 

by Jose Ignacio Galarza, 13 years ago

Attachment: patch.2.diff added

comment:7 by Jose Ignacio Galarza, 13 years ago

Yes, you are right!

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

Last edited 13 years ago by Jose Ignacio Galarza (previous) (diff)

by Jose Ignacio Galarza, 13 years ago

Attachment: patch2.diff added

Better patch

comment:8 by Preston Holmes, 13 years ago

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 by Jose Ignacio Galarza, 13 years ago

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 :)

by Jose Ignacio Galarza, 13 years ago

Attachment: patch3.diff added

Patch with more test functions

comment:10 by Preston Holmes, 13 years ago

milestone: 1.4
Triage Stage: AcceptedReady for checkin

Latest patch look good

comment:11 by Jacob, 13 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:12 by Aymeric Augustin, 13 years ago

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 by Aymeric Augustin, 13 years ago

Resolution: fixed
Status: assignedclosed

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 by James Reynolds, 13 years ago

Cc: eire1130@… added
Version: master1.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 by Simon Charette, 13 years ago

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

See this page for more information concerning backport policy.

Last edited 13 years ago by Simon Charette (previous) (diff)

comment:16 by Anssi Kääriäinen, 13 years ago

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

Note: See TracTickets for help on using tickets.
Back to Top