Opened 13 years ago

Closed 11 years ago

#16663 closed Bug (needsinfo)

Model field choices should accept empty iterator

Reported by: Daniel Naab Owned by: nobody
Component: Database layer (models, ORM) Version: 1.3
Severity: Normal Keywords:
Cc: Daniel Naab Triage Stage: Design decision needed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The base Field class constructor uses the following shorthand to initialize the choices for a field (line 96 of django/db/model/fields/__init__.py):

self._choices = choices or []

In Python, an empty iterator evaluates to False, so if we did:

class MyModel(models.Model):
    my_field = models.IntegerField(choices=my_list_like_object)

_choices will default to an empty list if the my_list_like_object is permanently or temporarilly empty at time of class definition. However, the documentation clearly states the intent of allowing any iterable as a choices kwarg:

Finally, note that choices can be any iterable object -- not necessarily a list or tuple. This lets you construct choices dynamically. But if you find yourself hacking choices to be dynamic, you're probably better off using a proper database table with a ForeignKey. choices is meant for static data that doesn't change much, if ever.

So I propose making a small change to the above line:

if choices == None:
    self._choices = []
else:
    self._choices = choices

Attachments (2)

16663-failing-test.patch (2.2 KB ) - added by Aymeric Augustin 13 years ago.
16663-docs.patch (1.1 KB ) - added by Aymeric Augustin 13 years ago.

Download all attachments as: .zip

Change History (8)

comment:1 by Daniel Naab, 13 years ago

Cc: Daniel Naab added

comment:2 by Aymeric Augustin, 13 years ago

I respectfully disagree with this sentence:

In Python, an empty iterator evaluates to False

To the best of my knowledge, an iterator always evaluate to True, no matter if it's empty:

>>> bool(iter(()))
True

However, at first sight, the intent here is to use None instead of [] as the default value for choices, to avoid the usual mutable-default-value issue. Hence, a strict check against None looks more appropriate.


I began working on a patch (first patch attached)... and failed horribly.

In fact, if we want to handle properly the case when an empty list is passed in the choices argument, we mustn't set self._choices = [] when the choices argument isn't passed. Otherwise we can no longer tell if (a) no choices were given or (b) a empty list was given, that will be populated later. This becomes ugly very quickly because Django uses if <field>.choices in many places. I'm afraid a lot of third-party code (like custom fields) also uses this idiom.

In particular, Django only creates get_FOO_display when <field>.choices evaluates to True when the field is initialized, which is why the test in my patch fails.


So, even if we implement a strict check (self._choices = [] if choices is None else choices), it still won't be possible to use an empty list as initial value of choices. Fixing this properly is backwards-incompatible. So I recommend not changing anything :)

Since iterators always evaluate to True, this shouldn't affect too many people. We could put a note in the docs (second patch attached, it just adds the word "non-empty" and rewraps the paragraph).

Last edited 13 years ago by Aymeric Augustin (previous) (diff)

by Aymeric Augustin, 13 years ago

Attachment: 16663-failing-test.patch added

by Aymeric Augustin, 13 years ago

Attachment: 16663-docs.patch added

comment:3 by Aymeric Augustin, 13 years ago

Easy pickings: unset
Has patch: set
Triage Stage: UnreviewedDesign decision needed

comment:4 by Daniel Naab, 13 years ago

I suspected there would be side effects of making the default self._choices == None instead of the empty list, which is why I proposed the simpler solution. A list-like object with length of zero is functionally identical to an empty list, so I don't see what possible side effects there could be to my suggestion (although setting choices to None is more sensible in an absolute sense).

comment:5 by Jacob, 13 years ago

danielnaab, can you clarify the real-world usecase here? I'm having trouble figuring out if this is actually a bug or something more abstract. Thanks!

comment:6 by Aymeric Augustin, 11 years ago

Resolution: needsinfo
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top