Code

Opened 6 years ago

Closed 3 years ago

Last modified 3 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: master
Severity: Keywords: DecimalField
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

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 3 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 3 years ago.
Patch to the documentation.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 6 years ago by mtredinnick

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 Changed 3 years ago by elbarto

  • Owner changed from nobody to elbarto

comment:3 Changed 3 years ago by elbarto

  • 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 Changed 3 years ago by ramiro

  • Needs documentation set
  • Needs tests set

Changed 3 years ago by elbarto

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

comment:5 Changed 3 years ago by elbarto

  • Needs tests unset

Changed 3 years ago by elbarto

Patch to the documentation.

comment:6 Changed 3 years ago by elbarto

  • Needs documentation unset

comment:7 Changed 3 years ago by ramiro

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

(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 Changed 3 years ago by ramiro

(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.

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.