Opened 8 years ago

Closed 8 years ago

#25731 closed Cleanup/optimization (fixed)

Remove choices from ether instance or method attribute in Select widget

Reported by: Johannes Maron Owned by: nobody
Component: Forms Version: 1.8
Severity: Normal Keywords:
Cc: James Pic Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

django.forms.widgets.Select chains self.choices a kwarg choices.

I think this is a bit over the top, and results in a lot of complexity. I couldn't find a good reason to why this is actually implemented that way.

I'd like to drop one of the two. Ether remove the choices from the instance and keep passing them in all methods, or preferable remove them from the method signatures.
The latter I think is cleaner but does include a bit of deprecation.

Cheers,
Joe

Change History (9)

comment:1 by Tim Graham, 8 years ago

It's a bit difficult for me to evaluate the ticket without some working code. Upon working on it, you might find the reason for the current implementation. Can you provide a patch?

comment:2 by Johannes Maron, 8 years ago

Tim, I could work on a patch, but I'd like to make a design decision first.

As I see it, the only reason why there is the option to pass choices into the renderer directly is the ModelChoiceIterator.
Where the regular choices get cast into a list, this would be a very poor idea for the ModelChoiceIterator.

The question is, should we allow Widgets to be rendered multiple times and drop the instance attribute, or should we not.
Another option would be to deepcopy the choices when they're being rendered.

comment:3 by Tim Graham, 8 years ago

I think it's very likely that some users render widgets multiple times on a page for whatever reason, so I don't think removing that functionality is an option.

comment:4 by Tim Graham, 8 years ago

Resolution: needsinfo
Status: newclosed

Please reopen if you can provide a patch, otherwise I'm not sure how to proceed with this ticket.

comment:5 by James Pic, 8 years ago

Agreed that this part is a bit fuzzy, seems like a leftover from before a refactor.

I do not see any valid use case for this argument, why would a user put so much effort into appending options that won't validate to a select widget ?

Patch proposed: https://github.com/django/django/pull/6037

comment:6 by James Pic, 8 years ago

Cc: James Pic added

comment:7 by Tim Graham <timograham@…>, 8 years ago

In 65c13f96:

Refs #25731 -- Removed unused MultipleHiddenInput choices

comment:8 by Tim Graham, 8 years ago

Has patch: set
Resolution: needsinfo
Status: closednew
Triage Stage: UnreviewedAccepted

comment:9 by Tim Graham <timograham@…>, 8 years ago

Resolution: fixed
Status: newclosed

In 926e9013:

Fixed #25731 -- Removed unused choices kwarg for Select.render()

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