Code

Opened 3 years ago

Closed 2 years ago

Last modified 23 months 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 23 months 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 23 months 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 23 months ago by charettes (previous) (diff)

comment:16 Changed 23 months 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.