list_display does not allow foreign keys __ syntax
Anshuman Aggarwal
I did look in the list and have tried taking this discussion to the ticket system. http://code.djangoproject.com/ticket/5863
In a nutshell, list_display does not accept foreign key __ syntax which is being used everywhere (including list_filter as of 1.3)...Just because callables exist as an advanced way of doing custom items in list does not take away from the argument that __ fk is a very very common requirement in the list display.
Here are the options currently available:
1. Write functions everywhere (this involves a lot of repetition for every single foreign field)
2. Write a wrapper like @lukeplant suggested in #5863 (even though it is far superior solution to individual functions in every model)
These are all HUGE violation of DRY principles on which sensible programming is based toady.
Most model fields already have a verbose_name, admin_sort_order etc. defined ...why should it be required to specify in just boilerplate code all of this all over again? Even in the wrapper function by @lukeplant, this verbose_name cannot be retrieved from the related model.
Since the parent__child syntax is being used for list filter, search fields and everywhere else, is there a reason why the list_display has to be sacrosanct?
The real question is, will this be accepted as a patch if contributed on the trunk with test cases et all?
Regards,
Anshuman
Anshuman Aggarwal
Luke Plant
> Since the parent__child syntax is being used for list filter, search
> fields and everywhere else, is there a reason why the list_display has
> to be sacrosanct?
Please see my response on Trac - I didn't notice you had brought this up
here, so posted there.
Thanks,
Luke
--
Sometimes I wonder if men and women really suit each other. Perhaps
they should live next door and just visit now and then. (Katherine
Hepburn)
Luke Plant || http://lukeplant.me.uk/
Anshuman Aggarwal
marked closed).
I got a mail to not open tickets closed by the core team and to take
up the discussion here. I am putting my reply to your comment here and
keeping the discussion on this thread for resolution.
list_display operates on database fields for all model fields anyways
so its not restricted to python methods/functions.
Comment (by brillgen)
@lukeplant, the basic problem you've correctly raised is that
list_display
allows for callables and hence arbitrary names can be used which are
similar to the syntax for foreign key fields.
However, this problem exists for list_filter also: someone may define
a
field with the name class__field and try to use that in list_filter as
a
foreign key field. (i've just checked that you can indeed name a field
like that)
So we can't really be compensating for dev stupidity at the cost of
functionality as we haven't done in list_filter also.
Since all FKs always start with an alphabet are in the regex format
(.+__.+.*), the __unicode etc cases can be easily and correctly
filtered
as is required to be done in list_filter.
Comment (by lukeplant):
Also, in response to andybak and anonymous, please note:
list_display is fundamentally different from list_filter and
search_fields. The other two are defining operations that must happen
in
the database, whereas list_display is defining an operation that must
happen in Python code, and refers to Python functions/methods, and
**not**
to database fields. The confusion is that a name that of a database
field
is also the name of the corresponding attribute on the Python object,
but
that is where the similarity ends.
So, for instance, you can set `'__unicode__'` in list_display, and it
will
refer to the `__unicode__` method, and not attempt any field lookups,
despite the presence of double underscores. As it happens, this
example
makes it obvious that we cannot sensibly add the feature as proposed -
how
would you refer to the `__unicode__` method on a related object?
`'foreign_key____unicode__'`? How would you parse that? What about
other
methods that people might choose to define, like `__html__`, etc.? The
only way this feature could get in is we are happy with a bunch of
special
cases in the implementation, and a bunch of arbitrary limitations in
functionality.
Luke Plant
On 17/05/11 14:42, Anshuman Aggarwal wrote:
> list_display
> allows for callables and hence arbitrary names can be used which are
> similar to the syntax for foreign key fields.
> However, this problem exists for list_filter also: someone may define
> a
> field with the name class__field and try to use that in list_filter as
> a
> foreign key field. (i've just checked that you can indeed name a field
> like that)
>
> So we can't really be compensating for dev stupidity at the cost of
> functionality as we haven't done in list_filter also.
> Since all FKs always start with an alphabet are in the regex format
> (.+__.+.*), the __unicode etc cases can be easily and correctly
> filtered
> as is required to be done in list_filter.
OK, back to basics:
ModelAdmin.list_display answers this question:
1) Given a Python object that is a Model instance, what pieces of
information should we display about it in the admin change list?
It answers the question in this way:
We display string representations of A) attributes of the instance,
which can be defined by strings and B) where that does not suffice,
arbitrary callables, which can be defined by strings (referring to
ModelAdmin attributes), or can be actual callables.
However, once you've answered that question, you've got another:
2) Given only a ModelAdmin and/or Model class, how can we define
*titles* (i.e. column headers) for the pieces of information defined by
list_display?
The answer to this is harder, because in general we can't. However, we
can make some pretty good guesses that will cover 95%. These include:
A) if the piece of information is an attribute that is a field on the
class, use metadata from the Model field to create the title i.e.
Field.verbose_name.
This is a very good guess, because we can be pretty sure that the
attribute will correspond to the field.
B) if the attribute is '__str__', use the verbose_name of the Model
Failing this:
C) Just turn 'some_attribute' into 'Some attribute'
and then:
D) Allow complete customisation via the 'short_description'
functionality
Given this is what list_display is about, the suggestion of supporting
'__' like in list_filter is just the wrong thing, because that syntax
has to do with traversing model relationships. Given question (1), which
is the primary question, and the answer we've come up with so far, a
much more appropriate suggestion would be to use dotted path syntax:
'some_obj.some_field'
Using '__' only makes sense for answering question 2, and only for case
2.A, and this is the reason we end up with silly things like
'foo____str__', which, even if we could get it work robustly, is
extremely ugly.
Then comes the question - should we support the dot syntax? This is
sensible for answering Q1, but not for Q2, because you can't go from
attributes to fields in general. We could, perhaps, support dot syntax
and attempt to do field lookups, but then, firstly, we've got some
implementation problems:
We will either have to
- duplicate some of the logic from the ORM about traversing fields,
adding in the attribute lookups.
- OR put logic into the core ORM that is there to support this
admin functionality.
(This applies whether we use '__' or '.' syntax).
Both of these are big code smells.
Secondly, we also have a question of what we do this the field
information once we've got it. Suppose we have these models:
class Place(Model):
short_name = CharField()
realm = CharField('Country')
class Person(Model):
full_name = CharField()
level = CharField() # e.g. BA, PhD
class Department(Model):
name = CharField()
manager = ForeignKey(Person)
location = ForeignKey(Place)
And then:
class DepartmentAdmin(ModelAdmin):
list_display = ['name',
'manager.full_name',
'manager.level',
'location.short_name',
'location.realm'
]
What should the display columns be called? I think I would want:
Name | Manager | Manager level | Location | Country
3 different rules would be required to satisfy all these, and all rules
give really bad results in some cases. Once you add i18n in, some rules
get completely impractical.
The success rate for this kind of guessing can't be much better than
50%, and that is just not worth it - we've already got a 95% solution,
with a method for handling the 5%, so I cannot see that adding a lot of
fragile machinery to get to a 97% solution is going to be worth it.
I could just about see a case for supporting the dotted syntax and not
attempting anything clever to answer Q2, but given the problems
highlighted above, you are very often going to need to use a callable
and 'short_description' to fix things, so I'm still unconvinced it is
worth it. If you have a problem with hundreds of fields that follow a
pattern, I'd suggest using a solution similar to the one I posted on the
ticket.
Best regards,
Anshuman Aggarwal
Thanks for a detailed reply. I have gone over your points carefully
and agree on most except a few key ones.
Based on your response:
- The question here is really how often is the guesswork right and
for how many different projects/use cases.
I have overseen Django used at more than a few projects and my
observation is that the guesswork logic that you described that is
presently followed for the headers will work very well (~75%) for FK
fields (i.e. take short description/verbose_name/name of field itself)
from the final model.
I realize that this may not be perfect and for advanced cases a
fallback to callables would be required. its anybody guess without
analyzing every single django project out there to say how many people
would benefit.
All in all, without this estimation maybe not worth the effort you
described building the . syntax
I have an alternate suggestion:
Can we develop the accessor callable that you described to be able to
access the short descrition/verbose_name etc and make it available as
a shortcut/decorator in the admin? With that, it won't complicate code
for the main admin, while keeping a quickie around for others??
I have taken your example and developed it a bit (though it still
violates DRY IMHO), but was wondering how the admin can pass in a
parameter with the model description so that it can be traversed to
obtain the field verbose_name/short_description parameters
def ForeignFieldFunc(field_name, short_description=None,
admin_order_field=None):
def accessor(obj):
val = obj
for part in field_name.split('__'):
val = getattr(val, part)
return val
if short_description:
accessor.short_description = short_description
else:
accessor.__name__ = field_name
if admin_order_field:
accessor.admin_order_field = admin_order_field
else:
accessor.admin_order_field = (field_name,)
return accessor
Regards,
Anshuman
Anshuman Aggarwal
short_description='FK Field') but ideally, the short_description
should be optional with the default logic being used to fetch it out
of the FK.
We can change the __ to a . easily enough without any impact to
anything else.
Anshuman Aggarwal
Luke Plant
> Hate to bump a thread, but any thoughts on the last proposal, Luke?
My personal opinion is that the feature you proposed is not going to be
correct enough of the time to warrant the duplication of code that it
would require to work.
Other core devs may have a different opinion, I don't know.
Luke
--
"Trouble: Luck can't last a lifetime, unless you die young."
(despair.com)
Luke Plant || http://lukeplant.me.uk/
Russell Keith-Magee
> On 25/05/11 10:55, Anshuman Aggarwal wrote:
>> Hate to bump a thread, but any thoughts on the last proposal, Luke?
>
> My personal opinion is that the feature you proposed is not going to be
> correct enough of the time to warrant the duplication of code that it
> would require to work.
>
> Other core devs may have a different opinion, I don't know.
I concur with Luke's analysis. There is a good reason why double
underscore notation isn't appropriate for list_display, and there is
already an elegant way to handle the problem. We don't gain anything
by providing a second approach that will only work some of the time,
crammed into an obscurely named inline function call.
Yours,
Russ Magee %-)