Code

Opened 7 years ago

Closed 3 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 6 years ago.
empty_frozen_set_raises_validation_error.patch (886 bytes) - added by lomin 4 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 7 years ago by Simon G. <dev@…>

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from newforms: [] not in EMPTY_VALUES to [] not in EMPTY_VALUES in newforms
  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from SVN to newforms branch

The thread in Django-Developers is here.

comment:2 Changed 7 years ago by adrian

  • Version changed from newforms branch to SVN

comment:3 Changed 7 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 7 years ago by tvrg

also see #5828

comment:5 Changed 6 years ago by mdnesvold

  • Owner changed from nobody to mdnesvold
  • Status changed from new to assigned
  • Summary changed from [] not in EMPTY_VALUES in newforms to [] not in EMPTY_VALUES in forms
  • Version changed from SVN to 1.0

Changed 6 years ago by mdnesvold

comment:6 Changed 6 years ago by mdnesvold

  • Has patch set

comment:7 Changed 6 years ago by mdnesvold

  • Needs tests set
  • Triage Stage changed from Accepted to Ready for checkin

comment:8 Changed 6 years ago by kmtracey

  • Triage Stage changed from Ready for checkin to 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 Changed 4 years ago by adamnelson

  • Patch needs improvement set

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

Changed 4 years ago by lomin

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

comment:10 Changed 3 years ago by lrekucki

  • Severity set to Normal
  • Type set to Bug

comment:11 Changed 3 years ago by claudep

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

  • Easy pickings unset
  • Resolution set to fixed
  • Status changed from assigned to 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.

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.