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