Opened 3 years ago

Closed 2 years ago

#33028 closed Cleanup/optimization (fixed)

ModelAdmin.init has self.opts, but it still nowerewhere else in django.admin.contrib.options not used

Reported by: Maxim Danilov Owned by: Marcelo Galigniana
Component: contrib.admin Version: 3.2
Severity: Normal Keywords: admin, modeladmin
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

At thirst:

ModelAdmin.__init__():
        self.model = model
        self.opts = model._meta

after that i see it more than 40 times in django.admin.contrib.options.py
...

 def __str__(self):
        return "%s.%s" % (self.model._meta.app_label, self.__class__.__name__)  # r.n. 593

...

info = self.model._meta.app_label, self.model._meta.model_name  # r.n. 620

It's easy to change with find/replace

Change History (13)

comment:1 by Maxim Danilov, 3 years ago

Owner: changed from nobody to Maxim Danilov
Status: newassigned
Type: UncategorizedCleanup/optimization

comment:2 by Carlton Gibson, 3 years ago

Triage Stage: UnreviewedAccepted

Hi Maxim.

I'll provisionally accept this for review. Let's see what the cleanup looks like.

This have been the same for a long time. (e.g. Many of the lines are unchanged since a19ed8aea395e8e07164ff7d85bd7dff2f24edca.) We can't change the API here, and it might be that it's not worth it looking at the diff, but in principle yes, a bit more consistency of usage might make it easier to follow.

Thanks

comment:3 by Marcelo Galigniana, 3 years ago

Owner: changed from Maxim Danilov to Marcelo Galigniana

Hello!

I can do the find/replace to see how the diff looks like.

But I would suggest to do the opposite way: remove the use of .opts and use ._meta instead (which in my opinion it's easier; at least to get more consistency). What do you think?

I did a quick search in the project and I got:

  • For ".opts" -> 66 results in 11 files
  • For "model._meta" -> 980 results in 134 files
  • And just "._meta" -> 2215 results in 225 files

Update: I didn't want unassign Maxim, I just saw the "4 months ago" and I thought it was unassigned, sorry!

Last edited 3 years ago by Marcelo Galigniana (previous) (diff)

comment:4 by Maxim Danilov, 3 years ago

Owner: changed from Marcelo Galigniana to Maxim Danilov

Hi Marcelo.
If i use ModelAdmin.opts, i use public Attribute from instance and i can override it for my needs.
i think it is right.

If in ModelAdmin placed self.model._meta, at first it is wrong to use privat property from other object, at second - it is possible, but more difficult to override it.

That's why i see ModelAdmin.opts is a right way for Django.


in reply to:  4 comment:5 by Marcelo Galigniana, 3 years ago

Thank you for the explanation Maxim! And sorry again!

Replying to Maxim Danilov:

Hi Marcelo.
If i use ModelAdmin.opts, i use public Attribute from instance and i can override it for my needs.
i think it is right.

If in ModelAdmin placed self.model._meta, at first it is wrong to use privat property from other object, at second - it is possible, but more difficult to override it.

That's why i see ModelAdmin.opts is a right way for Django.


comment:6 by Mariusz Felisiak, 2 years ago

Owner: Maxim Danilov removed
Status: assignednew

comment:7 by Marcelo Galigniana, 2 years ago

Owner: set to Marcelo Galigniana
Status: newassigned

comment:8 by Marcelo Galigniana, 2 years ago

Has patch: set

comment:9 by Marcelo Galigniana, 2 years ago

comment:10 by Mariusz Felisiak, 2 years ago

Patch needs improvement: set

comment:11 by Marcelo Galigniana, 2 years ago

Patch needs improvement: unset

comment:12 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:13 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In d80a2585:

Fixed #33028 -- Used ModelAdmin's opts attribute instead of model._meta.

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