Opened 19 years ago
Closed 14 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 boolean Trueness 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 boolean 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 , 18 years ago
| Version: | newforms branch → SVN |
|---|
comment:3 by , 18 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 Fields 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 , 17 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 , 17 years ago
| Attachment: | empty_values.diff added |
|---|
comment:6 by , 17 years ago
| Has patch: | set |
|---|
comment:7 by , 17 years ago
| Needs tests: | set |
|---|---|
| Triage Stage: | Accepted → Ready for checkin |
comment:8 by , 17 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 , 16 years ago
| Patch needs improvement: | set |
|---|
The patch doesn't have tests and doesn't include things like frozenset()
by , 15 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 , 15 years ago
| Severity: | → Normal |
|---|---|
| Type: | → Bug |
comment:11 by , 15 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 , 14 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.