Opened 18 years ago
Closed 13 years ago
#4051 closed Bug (fixed)
[] not in EMPTY_VALUES in forms
Reported by: | Owned by: | mdnesvold | |
---|---|---|---|
Component: | Forms | Version: | 1.0 |
Severity: | Normal | Keywords: | |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
the EMPTY_VALUES
list of newforms, which is used to test emptyness of required fields, lists ''
and None
, but not []
, which is counter-intuitive for list-like fields and requires those to implement their own emptyness check instead of relying of their parent class's checks. an example of this is the MultipleChoiceField
(source:django/trunk/django/newforms/fields.py line 376).
this could potentially be fixed by checking bool
ean True
ness instead of comparing to a list of known false values, although we would have to solve the problem of int(0)
and float(0)
, which are bool
ean False
but not considered omitted in most cases, by catching it in a separate NONEMPTY_VALUES=[0]
, which would diminish the elegancy of that solution.
(References: <461FD3E4.8040109@…> in the django-development list)
Attachments (2)
Change History (14)
comment:1 by , 18 years ago
Summary: | newforms: [] not in EMPTY_VALUES → [] not in EMPTY_VALUES in newforms |
---|---|
Triage Stage: | Unreviewed → Accepted |
Version: | SVN → newforms branch |
comment:2 by , 17 years ago
Version: | newforms branch → SVN |
---|
comment:3 by , 17 years ago
But the fields that do take in a list of values (MultiValueField
and MultipleChoiceField
at least) have clean
methods that must specially handle the lists they are given since they must process each item in the list. Now, there might be a way to factor out some commonalities of Field
s that take a list or commonalities of ChoiceField.clean
and MultipleChoiceField.clean
so that these clean
methods could call their parents.
Although, I guess I'm mainly talking about MultipleChoiceField
above since it would require some changes to work with its parent, ChoiceField.clean
. MultiValueField
doesn't have a problem though and could readily call its parent if []
was in EMPTY_VALUES
. There would be a bit of redundancy there since MultiValueField.clean
is already performing a boolean test on the passed value, but that could easily be fixed.
It seems that end goal is to have the methods be calling their parents though, a worthy goal I would say so that if things ever changed up the class hierarchy it wouldn't have to also be fixed on the classes who aren't calling their parents. But, it doesn't look like simply adding []
to EMPTY_VALUES
is going to solve this completely.
comment:5 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Summary: | [] not in EMPTY_VALUES in newforms → [] not in EMPTY_VALUES in forms |
Version: | SVN → 1.0 |
by , 16 years ago
Attachment: | empty_values.diff added |
---|
comment:6 by , 16 years ago
Has patch: | set |
---|
comment:7 by , 16 years ago
Needs tests: | set |
---|---|
Triage Stage: | Accepted → Ready for checkin |
comment:8 by , 16 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
If it needs tests it's not ready for checkin. Also "Ready for checkin" should not be set by the person who provides the patch, it's meant to indicate an independent reviewer has looked the patch over and agrees all is set to go. See: http://docs.djangoproject.com/en/dev/internals/contributing/#ticket-triage
comment:9 by , 15 years ago
Patch needs improvement: | set |
---|
The patch doesn't have tests and doesn't include things like frozenset()
by , 14 years ago
Attachment: | empty_frozen_set_raises_validation_error.patch added |
---|
Extends the test of MultipleChoiceField by the case of an empty frozenset
comment:10 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → Bug |
comment:11 by , 14 years ago
Initial bug is now almost fixed, except that empty set/frozenset aren't considered in EMPTY_VALUES. Does that make sense? Are there use cases for widgets returning set? AFAIK MultipleSelect always returns a list.
comment:12 by , 13 years ago
Easy pickings: | unset |
---|---|
Resolution: | → fixed |
Status: | assigned → closed |
UI/UX: | unset |
If there are use cases for empty set/frozenset in EMPTY_VALUES, someone will probably open a new report. Closing this one.
The thread in Django-Developers is here.