Opened 2 years ago

Closed 2 years ago

#33258 closed Bug (needsinfo)

inconsistent use of () and [] for attributes in ModelAdmin class - Also class variables

Reported by: Martin Massera Owned by: nobody
Component: contrib.admin Version: 3.2
Severity: Normal Keywords: Admin
Cc: Claude Paroz Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Martin Massera)

In Django's ModelAdmin class, most "lists" are initialized as tuples () while two of them (inlines and actions)are initialized as lists []

list_display = ('__str__',)
list_display_links = ()
list_filter = ()
...
list_editable = ()
search_fields = ()
...
inlines = []
...
actions = []

This is inconsistent.

And also since these are declared in the class, they are shared among all instances if this value is not set. This brings the problem where you inadvertedly add a value to a list and this gets added to all Admin instances where this attribute has not been set.

ie: I wanted to add an action to an admin subclass so I did MyAdmin.actions.append('some_action') which added this action to all my admins who had not set a new value to actions.

While lists are more semantically correct than tuples, tuples have the advantage of being immutable, so they force users to reset the value every time.

So, two solutions:

  • declare all these attributes as instance variables in the constructor. Possibly setting them all as lists
  • declare all these attributes as tuples, keeping them where they are

Change History (5)

comment:1 by Martin Massera, 2 years ago

Description: modified (diff)

comment:2 by Martin Massera, 2 years ago

Description: modified (diff)

comment:3 by Martin Massera, 2 years ago

Description: modified (diff)
Summary: inconsistent use of () and [] for attributes in Admin class - Also class variablesinconsistent use of () and [] for attributes in ModelAdmin class - Also class variables

comment:4 by Carlton Gibson, 2 years ago

Cc: Claude Paroz added

OK, I think this needs to go to the DevelopersMailingList.

There was some movement towards lists (not necessarily in these exact locations) in #30947 and PR 11267.

The idea was the homogenous lists should lists, where other iterables where the fields were of different kinds should use tuples.

(I defer the question of which applies where in the case raised here.)

And also since these are declared in the class, they are shared among all instances if this value is not set.
...
While lists are more semantically correct than tuples, tuples have the advantage of being immutable, so they force users to reset the value every time.

This seems more pressing. Claude raise this worry in the discussion on the commit for the PR 11267

... I think we should consider the impact on mutability too. Don't we get a new risk here where people might suddenly change a class-level list ...

I'm not sure what the best way to balance the considerations is, but I think it's worth asking for input.

comment:5 by Carlton Gibson, 2 years ago

Resolution: needsinfo
Status: newclosed

I'll mark this needs info pending a discussion.

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