Opened 9 years ago

Closed 5 years ago

#4051 closed Bug (fixed)

[] not in EMPTY_VALUES in forms

Reported by: webograph <webograph@…> 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)

empty_values.diff (387 bytes) - added by mdnesvold 8 years ago.
empty_frozen_set_raises_validation_error.patch (886 bytes) - added by lomin 6 years ago.
Extends the test of MultipleChoiceField by the case of an empty frozenset

Download all attachments as: .zip

Change History (14)

comment:1 Changed 9 years ago by Simon G. <dev@…>

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Summary: newforms: [] not in EMPTY_VALUES[] not in EMPTY_VALUES in newforms
Triage Stage: UnreviewedAccepted
Version: SVNnewforms branch

The thread in Django-Developers is here.

comment:2 Changed 9 years ago by Adrian Holovaty

Version: newforms branchSVN

comment:3 Changed 9 years ago by Gary Wilson <gary.wilson@…>

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:4 Changed 9 years ago by Tom Vergote

also see #5828

comment:5 Changed 8 years ago by mdnesvold

Owner: changed from nobody to mdnesvold
Status: newassigned
Summary: [] not in EMPTY_VALUES in newforms[] not in EMPTY_VALUES in forms
Version: SVN1.0

Changed 8 years ago by mdnesvold

Attachment: empty_values.diff added

comment:6 Changed 8 years ago by mdnesvold

Has patch: set

comment:7 Changed 8 years ago by mdnesvold

Needs tests: set
Triage Stage: AcceptedReady for checkin

comment:8 Changed 8 years ago by Karen Tracey

Triage Stage: Ready for checkinAccepted

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 Changed 6 years ago by Adam Nelson

Patch needs improvement: set

The patch doesn't have tests and doesn't include things like frozenset()

Changed 6 years ago by lomin

Extends the test of MultipleChoiceField by the case of an empty frozenset

comment:10 Changed 6 years ago by Łukasz Rekucki

Severity: Normal
Type: Bug

comment:11 Changed 6 years ago by Claude Paroz

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 Changed 5 years ago by Claude Paroz

Easy pickings: unset
Resolution: fixed
Status: assignedclosed
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.

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