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 , 3 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
Type: | Uncategorized → Cleanup/optimization |
comment:2 by , 3 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:3 by , 3 years ago
Owner: | changed from | to
---|
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 self.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 my local 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
follow-up: 5 comment:4 by , 3 years ago
Owner: | changed from | to
---|
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:5 by , 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 , 2 years ago
Owner: | removed |
---|---|
Status: | assigned → new |
comment:7 by , 2 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:8 by , 2 years ago
Has patch: | set |
---|
comment:10 by , 2 years ago
Patch needs improvement: | set |
---|
comment:11 by , 2 years ago
Patch needs improvement: | unset |
---|
comment:12 by , 2 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
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