[Django] #33931: Optimize calling of `get_app_list` with AdminSites index/app_index
Django
------------------------------------------------+------------------------
Reporter: Daniel Hahler | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 4.1
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
`AdminSite.index` gets the list of available apps twice: for `app_list`
and `available_apps` in the context.
I think this can be optimized to only call it once.
With `app_index` it could also be filtered in `app_index` itself.
The benefit of this is handling the use case of moving apps around in
`get_app_list` (from `custom_auth` into `auth`, after calling the default
method), which fails when the list is filtered by `app_label` before.
I think changing `index` makes sense, and is the far more often used code
path, but changing `app_index` might break some projects, where by now it
might be expected that `get_app_list` gets called with an `app_label` -
and optimizing it there removed the internal use of calling `get_app_list`
with `app_label` completely again. (This was changed in Django 4.1:
https://code.djangoproject.com/ticket/7497)
--
Ticket URL: <https://code.djangoproject.com/ticket/33931>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Daniel Hahler | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 4.1
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
* stage: Unreviewed => Accepted
Comment:
Hey Daniel.
I think at least for the `index` case it does make sense. My only
reservation there is whether the micro-optimisation justifies the change
in readability: `get_app_list()` is clear as day, whereas I might need to
dig down to see why we're doing `"app_list": context["available_apps"]`. A
comment there in the two cases would be enough I guess, `Aliasing for the
template context`, (or not).
The `app_index` case is more complex. It looks like maybe the `app_label`
usage could be teased out/simplified, but it's clearly old/delicate. I
wonder how much effort you want to put in here? Do you want to deprecate
the (4.1 added) `app_label` argument, as unused?
What about (then) simplifying `_build_app_dict()` — the `label` parameter
would be unused, and there's a block of code using that that could be
removed? If we're going to do this, I think we should clean that up too?
If you're happy to take that on, then happy to progress to see what others
think, given that `_build_app_dict()` is somewhat complex (to read at
least). Otherwise we can `wontfix` as it's likely marginal.
> The benefit of this is handling the use case of moving apps around in
get_app_list (from custom_auth into auth, after calling the default
method), which fails when the list is filtered by app_label before.
I'd imagine a bit of defensive programming would be enough here no? 🤔 But
maybe there's a slightly separate thought/improvement there?
--
Ticket URL: <https://code.djangoproject.com/ticket/33931#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: Daniel Hahler | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: contrib.admin | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by Carlton Gibson):
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/33931#comment:2>
Django
Reporter: Daniel Hahler | Owner: Daniel
Type: | Hahler
Cleanup/optimization | Status: assigned
Component: contrib.admin | Version: 4.1
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* owner: nobody => Daniel Hahler
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/33931#comment:3>