Opened 16 years ago

Closed 13 years ago

Last modified 13 years ago

#7726 closed (fixed)

DecimalField: Semantics of max_digits in combination with decimal_places confusing and perhaps wrong

Reported by: theevilgeek Owned by: elbarto
Component: Validators Version: dev
Severity: Keywords: DecimalField
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

"Problem"

In attempting to capture fixed point decimal values in the range of .0000 to .9999, I created a DecimalField as follows:

value = forms.DecimalField(max_digits=4, decimal_places=4)

However, it seems that any decimal submitted is first normalized to include a leading "0." should no other non-fractional
digits be provided.

The presence of this additional "0." causes the input to instead resemble 0.XXXX. However, since decimal_places = 4
and max_digits = 4, no input will successfully validate-- the leading zero simply hasn't been accounted for.

Possible solutions

  1. irc:Gulopine and irc:Magus suggest that this behavior is not a problem: a properly formatted decimal lacking a whole component incorporates a leading "0."; thus, the user should anticipate this extra digit and plan accordingly.

However, the documentation for the DecimalField (http://www.djangoproject.com/documentation/model-api/#decimalfield [treating the model and newforms fields as being interchangeable]) indicates that the field stores a "A fixed-precision decimal number". As fixed-precision (or fixed-point) decimal notation usually implies exacting control over the number of positions before and after the decimal point that will be incorporated in a representation of the data (http://en.wikipedia.org/wiki/Fixed-point_arithmetic), it seems reasonable to expect that the DecimalField will exactingly respect the max_digits and decimal_places arguments to the same effect. Thus, I feel that the exhibited behavior is, at the least, somewhat surprising.

  1. irc:Gulopine suggests that DecimalField might be altered to raise an ImproperlyConfigured error if max_digits <= decimal_places.


  1. The DecimalField documentation might be enhanced to explicitly state that a leading "0." will be tacked onto any decimal numbers lacking whole digits and that this leading zero must be accounted for when specifying max_digits and decimal_places.
  1. The behavior of DecimalField is altered such that max_digits and decimal_places are strictly enforced.

Finally, it is worth noting that the examples in the documentation for DecimalField only reinforce the expectation of strict enforcement:
"For example, to store numbers up to 999 with a resolution of 2 decimal places, you’d use: models.DecimalField(..., max_digits=5, decimal_places=2)"

Attachments (2)

patch.diff (2.8 KB ) - added by elbarto 13 years ago.
I've send a new patch with tests which returns 'OK' with invalid_models test.
doc_patch.diff (724 bytes ) - added by elbarto 13 years ago.
Patch to the documentation.

Download all attachments as: .zip

Change History (10)

comment:1 by Malcolm Tredinnick, 16 years ago

Triage Stage: UnreviewedAccepted

It should be a validation error if max_digits < decimal_places +1. That is, this would be an error detected when you run manage.py validate. That's all that's required here. There will be at least one digit (possibly an implicit zero) to the left of the decimal point.

A patch to add that check to the validation routines in django.core.management will be most welcome. Thanks for taking the time to work through this.

comment:2 by elbarto, 13 years ago

Owner: changed from nobody to elbarto

comment:3 by elbarto, 13 years ago

Has patch: set

I have run the validators tests and it returns OK.

However, because of it's my first patch to the code, I don't know if I have to write new tests.

comment:4 by Ramiro Morales, 13 years ago

Needs documentation: set
Needs tests: set

by elbarto, 13 years ago

Attachment: patch.diff added

I've send a new patch with tests which returns 'OK' with invalid_models test.

comment:5 by elbarto, 13 years ago

Needs tests: unset

by elbarto, 13 years ago

Attachment: doc_patch.diff added

Patch to the documentation.

comment:6 by elbarto, 13 years ago

Needs documentation: unset

comment:7 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: newclosed

(In [15094]) Fixed #7726 -- Added validation of max_digits and decimal_places options to DecimalField model field. Thanks theevilgeek for the report and elbarto for the patch.

comment:8 by Ramiro Morales, 13 years ago

(In [15095]) [1.2.X] Fixed #7726 -- Added validation of max_digits and decimal_places options to DecimalField model field. Thanks theevilgeek for the report and elbarto for the patch.

Backport of [15094] from trunk.

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