Rationale behind #4620 (Custom labels for choices in ModelChoiceField) wontfix?
Maykel Moya
to the ticket (reopening it as well) in response to jacob's comment.
mtredinnick kindly advised me using the list instead of reopening it:
----
> Yes, we're sure this is wontfix.
Could you please give a short rationale on this given that having
custom labels is not trivial as you did think before. The solution, as
SmileyChris explained, involves some code duplication (maybe more code
duplicated than the actual bits used for getting the custom label
itself)
I have a class Foo that have a user = ForeignKey(User) and would like
to choose users for Foo instances by their full name instead of by
their username. User here is the standard class from
django.contrib.auth and so it's outside my control.
The needing of displaying full names instead of usernames in a
ModelChoiceField for django.contrib.auth.models.User doesn't seem so
uncommon to me.
----
Regards,
maykel
[1] http://code.djangoproject.com/ticket/4620#comment:8
[2] http://code.djangoproject.com/ticket/4620#comment:6
David Cramer
__unicode__ just for behavior like this either way.
James Bennett
> I have a class Foo that have a user = ForeignKey(User) and would like
> to choose users for Foo instances by their full name instead of by
> their username. User here is the standard class from
> django.contrib.auth and so it's outside my control.
Well, ultimately it's a trade-off: adding new configuration options
inevitably makes code more complex and introduces new vectors for
bugs, so such things should be considered carefully against the fact
that it's pretty easy to write a ModelChoiceField subclass and
override the bit that spits out the choices.
--
"Bureaucrat Conrad, you are technically correct -- the best kind of correct."
Trevor Caira
> Well, ultimately it's a trade-off: adding new configuration options
> inevitably makes code more complex and introduces new vectors for
> bugs, so such things should be considered carefully against the fact
> that it's pretty easy to write a ModelChoiceField subclass and
> override the bit that spits out the choices.
to override the choices property, which requires writing the
_get_choices and _set_choices methods, and you need to write the
iterator that spits out the choices -- if just subclass
QuerySetIterator and override __iter__, expect to write about 16 lines
of code or more to implement this custom behavior.
Trevor Caira
Brian Rosner
> inevitably makes code more complex and introduces new vectors for
> bugs, so such things should be considered carefully against the fact
> that it's pretty easy to write a ModelChoiceField subclass and
> override the bit that spits out the choices.
I am a +1 on a configuration parameter since the alternative by
subclassing is a bit too involved for something fairly trivial. See my
post about how to do so:
http://oebfare.com/blog/2008/feb/23/overriding-modelchoicefield-labels/
--
Brian Rosner
http://oebfare.com
Joseph Kocherhans
I had to do this the other night, and it is sort of a pain in the ass.
I haven't looked at the proposed solution(s), but it seems like this
would be nicely solved by a callback that can be passed in to
ModelChoiceField's constructor. The callback would just be a function
that takes an item from the queryset, and spits out a choice tuple.
Another possibility could be to allow ModelChoiceField to take a
custom object in place of QuerySetIterator and possibly adding
something like an emit_choice(obj) method to it to make subclassing a
little easier.
As it stands right now, you have to override a method, redefine a
property (to make your new method work), and reimplement
QuerySetIteratior.
I'm not going to write the code or the tests, but I'd be happy to
provide feedback for someone who *is* willing. I'll also try to beat
the relevant objectors into submission. ;)
Joseph
Joseph Kocherhans
<jkoch...@gmail.com> wrote:
> On Thu, Feb 28, 2008 at 5:44 PM, Brian Rosner <bro...@gmail.com> wrote:
> >
> > > Well, ultimately it's a trade-off: adding new configuration options
> > > inevitably makes code more complex and introduces new vectors for
> > > bugs, so such things should be considered carefully against the fact
> > > that it's pretty easy to write a ModelChoiceField subclass and
> > > override the bit that spits out the choices.
> >
> > I am a +1 on a configuration parameter since the alternative by
> > subclassing is a bit too involved for something fairly trivial. See my
> > post about how to do so:
> >
> > http://oebfare.com/blog/2008/feb/23/overriding-modelchoicefield-labels/
>
> I had to do this the other night, and it is sort of a pain in the ass.
> I haven't looked at the proposed solution(s), but it seems like this
> would be nicely solved by a callback that can be passed in to
> ModelChoiceField's constructor. The callback would just be a function
> that takes an item from the queryset, and spits out a choice tuple.
And this is exactly what the patch for #4620 does. Sorry, next time
I'll be a good boy and know what I'm talking about. ;)
Joseph
Philippe Raoult
I'd juste like to add my +1 on #4620. I agree that there's a trade-off
between options and complexity but the patch looks fairly trivial WRT
the service provided.
Philippe
Jacob Kaplan-Moss
it's actually appreciably more difficult to change ModelChoiceField
labels than I'd thought. I'm going to reopen the ticket.
I'm not too happy with the callback idea in the patch right now -- any
other good ideas?
Jacob
Jacob Kaplan-Moss
> I'm not too happy with the callback idea in the patch right now -- any
> other good ideas?
Had a quick chat with Joseph about this, and here's the idea I came up
with. Just a sketch, but if someone wants to run with it and see where
it goes that would be cool.
So I see no reason for QuerySetIterator to exist -- nothing uses it
except ModelChoiceField._get_choices, but because it's there you have
to subclass both ModelChoiceField and QuerySetIterator, and that's
lame.
So I think we should merge QuerySetIterator into
ModelChoiceField._get_choices. Once that's done, it's pretty trivial
to make _get_choices perform a callback to get the label for a
particular choice -- something like::
def _get_choices(self):
...
for obj in self.queryset:
yield (obj.ok, self.label_for_object(obj))
...
Then subclasses would just need to define label_for_object -- which,
BTW, is a really shitty name. Whoever paints this particular bikeshed
should be sure to choose another color.
Jacob
Malcolm Tredinnick
On Fri, 2008-02-29 at 10:25 -0600, Jacob Kaplan-Moss wrote:
> On 2/29/08, Jacob Kaplan-Moss <jacob.ka...@gmail.com> wrote:
> > I'm not too happy with the callback idea in the patch right now -- any
> > other good ideas?
>
> Had a quick chat with Joseph about this, and here's the idea I came up
> with. Just a sketch, but if someone wants to run with it and see where
> it goes that would be cool.
>
> So I see no reason for QuerySetIterator to exist -- nothing uses it
> except ModelChoiceField._get_choices, but because it's there you have
> to subclass both ModelChoiceField and QuerySetIterator, and that's
> lame.
I was about 80% finished writing more or less the same solution over
here. Now I can save some typing and just say that I agree with you
guys. I got there via pretty much the same logic, too: subclassing
should be the solution, but it should require less keystrokes than it
does now.
Malcolm
--
Tolkien is hobbit-forming.
http://www.pointy-stick.com/blog/
Rob Hudson
> I was about 80% finished writing more or less the same solution over
> here. Now I can save some typing and just say that I agree with you
> guys. I got there via pretty much the same logic, too: subclassing
> should be the solution, but it should require less keystrokes than it
> does now.
If I can side-track the conversation for a moment and point to
something else I think should require a few less keystrokes to
subclass... The newforms widget render method.
Maybe I'm wrong, but it seems like all the attribute collecting could
be pushed into another method so that render() has a call to get_attrs
and simply returns the string. If I want to subclass widgets to make
HTML4 widgets (for example, in this snippet:
http://www.djangosnippets.org/snippets/618/), there are 4 lines of
code that are a straight cut and paste from the Input class' render()
method. (Or maybe there's a simpler way to do this?)
Thanks,
-Rob
Gary Wilson Jr.
Very much agree. +1
Gary
Jacob Kaplan-Moss
> If I can side-track the conversation for a moment [...]
Please, don't -- start another thread.
Jacob
Brian Rosner
> def _get_choices(self):
> ...
> for obj in self.queryset:
> yield (obj.ok, self.label_for_object(obj))
> ...
I wonder if this should be done in two methods so that iteration is
encapsulated away from queryset caching invalidation. Otherwise I am +1
on using subclassing to solve the problem.
Brian Rosner
<bro...@gmail.com> said:
>
>>
>> def _get_choices(self):
>> ...
>> for obj in self.queryset:
>> yield (obj.ok, self.label_for_object(obj))
>> ...
>
> I wonder if this should be done in two methods so that iteration is
> encapsulated away from queryset caching invalidation. Otherwise I am +1
> on using subclassing to solve the problem.
Ok, perhaps I should read the code before commenting. My bad :P