Opened 13 years ago

Closed 12 years ago

#16570 closed Bug (fixed)

allow DecimalFields with max_digits == decimal_places

Reported by: Dan Watson Owned by: kenth
Component: Database layer (models, ORM) Version: 1.3
Severity: Release blocker Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

This was discussed on django-dev here:

https://groups.google.com/d/topic/django-developers/TvMNbPWwetY/discussion

Essentially, the fix for #7726 caused a validation error for max_digits == decimal_places, which is a valid numeric type in the database.

Attachments (3)

16570.diff (2.9 KB ) - added by Dan Watson 13 years ago.
Patch against trunk r16579, all tests pass.
16570-2.diff (3.5 KB ) - added by Dan Watson 13 years ago.
Updated with documentation patch
16570-3.diff (3.6 KB ) - added by kenth 12 years ago.
Updated patch to account for refactoring in #16681

Download all attachments as: .zip

Change History (11)

by Dan Watson, 13 years ago

Attachment: 16570.diff added

Patch against trunk r16579, all tests pass.

comment:1 by Matthias Kestenholz, 13 years ago

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

The documentation needs to be changed, too:

.. attribute:: DecimalField.max_digits

    The maximum number of digits allowed in the number. Note that this number
    must be greater than ``decimal_places``, if it exists.

by Dan Watson, 13 years ago

Attachment: 16570-2.diff added

Updated with documentation patch

comment:2 by Dan Watson, 13 years ago

Patch needs improvement: unset

comment:3 by Malcolm Tredinnick, 13 years ago

Needs tests: set
Patch needs improvement: set

Feels like there should be a test included here. However, my reading of the original ticket we are modifying is that there was a problem with Django normalising .1234 to be 0.1234, so maybe a test is hard to write.

However, failing an argument that it's impossible to test at the Django level (and hence only relevant when Django creates the table), it needs a test to avoid future regressions.

comment:4 by Dan Watson, 13 years ago

I think the problem was that Django used to do it's own validation of decimals (see #7064 and the associated patch). It seems now, Django simply tries the Decimal constructor and catches any exception. Since Decimal('0.123') == Decimal('.123') (both get normalized to include the leading 0) I don't think this is an issue.

comment:5 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

comment:6 by kenth, 12 years ago

Owner: changed from nobody to kenth

by kenth, 12 years ago

Attachment: 16570-3.diff added

Updated patch to account for refactoring in #16681

comment:7 by kenth, 12 years ago

Needs tests: unset
Patch needs improvement: unset

comment:8 by Karen Tracey, 12 years ago

Resolution: fixed
Status: newclosed

In [17089]:

Fix #16570: Restore ability to have decimal fields where max_digits equals decimal_places. Thanks dcwatson and kenth.

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