#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
- 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.
- irc:Gulopine suggests that DecimalField might be altered to raise an ImproperlyConfigured error if max_digits <= decimal_places.
- 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.
- 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)
Change History (10)
comment:1 Changed 8 years ago by mtredinnick
- Needs documentation unset
- Needs tests unset
- Patch needs improvement unset
- Triage Stage changed from Unreviewed to Accepted
comment:2 Changed 6 years ago by elbarto
- Owner changed from nobody to elbarto
comment:3 Changed 6 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 6 years ago by ramiro
- Needs documentation set
- Needs tests set
Changed 6 years ago by elbarto
I've send a new patch with tests which returns 'OK' with invalid_models test.
comment:5 Changed 6 years ago by elbarto
- Needs tests unset
comment:6 Changed 6 years ago by elbarto
- Needs documentation unset
comment:7 Changed 6 years ago by ramiro
- Resolution set to fixed
- Status changed from new to closed
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.