Opened 12 years ago
Closed 12 years ago
#18829 closed Bug (fixed)
ModelChoiceIterator doesn't adjust its length to account for an empty item.
Reported by: | Owned by: | Klaas van Schelven | |
---|---|---|---|
Component: | Forms | Version: | 1.4 |
Severity: | Normal | Keywords: | ModelChoiceIterator ModelForm |
Cc: | Triage Stage: | Accepted | |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | yes | UI/UX: | no |
Description
In forms/models.py, the ModelChoiceIterator defines an __iter__
method which yields an empty entry if empty_label is not None
, but the __len__
method always returns the length of the queryset, no matter if the empty choice is present.
def __iter__(self): if self.field.empty_label is not None: yield (u"", self.field.empty_label) if self.field.cache_choices: if self.field.choice_cache is None: self.field.choice_cache = [ self.choice(obj) for obj in self.queryset.all() ] for choice in self.field.choice_cache: yield choice else: for obj in self.queryset.all(): yield self.choice(obj) def __len__(self): return len(self.queryset)
This can lead to erratic behavior when iterating. For example, the following template code wont hold the expected output, because the forloop.last will be True before for the second to last item, causing the semicolon to render on the last line and not in the previous one.
{% for choice_id, choice_name in my_form.my_field.field.choices %} {{ choice_id }} - {{ choice_name }} {% if not forloop.last %};{% endif %} {% endfor %}
This would produce an output like:
- -----; 1 - One; 2 - Two 3 - Three;
Attachments (1)
Change History (6)
comment:1 by , 12 years ago
Easy pickings: | set |
---|---|
Owner: | changed from | to
Status: | new → assigned |
by , 12 years ago
Attachment: | 18829.diff added |
---|
comment:2 by , 12 years ago
Has patch: | set |
---|
comment:3 by , 12 years ago
Summary: | ModelChoiceIterator returns the queryset length when → ModelChoiceIterator doesn't adjust its length to account for an empty item. |
---|---|
Triage Stage: | Unreviewed → Accepted |
comment:4 by , 12 years ago
Owner: | changed from | to
---|
I've added a test and bike-shedded the implementation; available as pull request on github
comment:5 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
The issue is valid. Personally, I don't like using booleans as ints, but apparently it's part of the spec, not an implementation detail [1], so I can't complain super much. Pretty sure this needs tests, though.
[1] http://stackoverflow.com/questions/2764017/is-false-0-and-true-1-in-python-an-implementation-detail-or-is-it-guarante