Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#14655 closed (fixed)

formsets should be iterable

Reported by: kenth Owned by: pandres
Component: Forms Version: master
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:


I have a inlineformset where I need to be able to display some of the "extra" forms (that I initialize) before some of the "initial" forms retrieved from the db. This would be easy if I could just override formset's iter method and get the forms to come out in an order different from the creation order. This patch adds this hook.

This patch also makes it possible to say "for form in formset" (instead of "for form in formset.forms") in both code & templates. There are no backward compatibility issues.

The pattern "for form in formset" is also closer to the "for field in form" idiom.

Attachments (4)

formset-iterable-docs.diff (6.6 KB) - added by kenth 5 years ago.
formset-iterable.diff (1.9 KB) - added by kenth 5 years ago.
formset-iterable-tests.diff (34.6 KB) - added by kenth 5 years ago.
formset-iterable-2.diff (1.7 KB) - added by kenth 5 years ago.
Improved patch delegating iter to self.forms

Download all attachments as: .zip

Change History (17)

comment:1 Changed 5 years ago by kenth

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Another use of __iter__ in formsets is to allow a formset panel of questions to be presented in random order. Override __iter__ to assign a random number to each form, and then display in sorted order.

comment:2 Changed 5 years ago by pandres

  • Owner changed from nobody to pandres

comment:3 Changed 5 years ago by pandres

  • Resolution set to wontfix
  • Status changed from new to closed

Your path seems to add another way to do something that is already easy to do (and it's done all around in the code). It goes against the principle of "there should be one obvious way to do it"

comment:4 Changed 5 years ago by russellm

  • Needs documentation set
  • Needs tests set
  • Resolution wontfix deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Unreviewed to Accepted

Although Pandres' comment is correct (i.e., there is already a way to iterate over forms), I think what you're proposing makes sense, both in terms of consistency in the API, and in terms of providing a point at which behavior can be modified.

Changed 5 years ago by kenth

comment:5 Changed 5 years ago by kenth

  • Needs documentation unset

Changed 5 years ago by kenth

Changed 5 years ago by kenth

comment:6 Changed 5 years ago by kenth

  • Needs tests unset

Added tests. As a result, I added getitem & len methods to the patch. Several of the tests indexed formset.forms.

Passes tests. Ready for review.

comment:7 Changed 5 years ago by ikelly

Instead of being a generator function, the __iter__ method should simply return iter(self.forms). Why add a layer of wrapping when you can delegate?

Why are the __getitem__ and __len__ methods necessary? Since they require iterating over the full formset, they are less efficient than the O(1) methods on the underlying list, which bugs me. Why not let them also delegate to self.forms, and anybody who needs to customize __iter__ should just customize those as well?

Changed 5 years ago by kenth

Improved patch delegating iter to self.forms

comment:8 Changed 5 years ago by kenth

@ikelly, thanks for your help. Duh on returning iter(self.forms). I too don't like the __getitem__ and __len__ methods, but they're required to pass the unit tests (mainly in modelforms). And since they're required, I implemented ones that worked correctly -- if slowly -- instead of requiring all three methods be overridden if __iter__ is. My formset which required reordering rendered forms (e.g. a sparse many-to-many relationship table of per-user ratings of beatles albums that I want to be rendered in album order, mixing "existing" and "extra" forms) would only require the __iter__ method. An inefficient but correct method for the (probably never used except in unit test) ancillary methods seemed like the way to go.

Of course, you improved my base __iter__ method. Any suggestions on the others would be great. I'd like to get the __iter__ hook into django. Python is not (yet) my best language.

comment:9 Changed 5 years ago by anonymous

Maybe not really needed but shouldn't the original way also be tested?
The patches change everything to this new method which is fine but maybe leave tests for the old way of doing it to make sure existing code doesn't get broken?

comment:10 Changed 5 years ago by kenth

The "original way" was to iterate over self.forms. The new way (iterating over self) also iterates over self.forms -- but via delegate per Ian's comments above. The only way to break the old way that I can think of would be to rename self.forms. I guess it could be tested for, I just didn't think it was needed. Is it?

comment:11 Changed 5 years ago by kenth

Also, many contrib modules continue to iterate over self.forms (like contrib.admin). I changed only the "core".

comment:12 Changed 5 years ago by russellm

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [14986]) Fixed #14655 -- Made formsets iterable. This allows a slightly more natural iteration API (for form in formsets), and allows you to easily override the form rendering order. Thanks to Kent Hauser for the suggestion and patch.

comment:13 Changed 5 years ago by russellm

I've just committed this, but a couple of comments for future reference:

  • We prefer patches to be submitted as a a single patch, not multiple partial patches. I was about to bounce this ticket due to a lack of documentation until I realized that documentation was contained in an different patch ticket.
  • You don't *ever* change an existing test case. Existing unittests are regressions for existing behavior. The fact that you're introducing a new behavior doesn't invalidate the existing behavior.
  • Code still needs to run under Python 2.4; this means you can't use context managers.
Note: See TracTickets for help on using tickets.
Back to Top