[Django] #24313: Deprecate the class_prepared signal
Django
------------------------------------------------+------------------------
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
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 |
------------------------------------------------+------------------------
Since 1.7, whatever can be done in a `class_prepared` listener is probably
better done by inspecting the app registry in `AppConfig.ready()`.
As the documentation notes, reacting to `class_prepared` is impractical
because the app-loading and meta-refactor APIs don't work yet.
Even if we keep the signal — I once tried to refactor this area and it
[https://code.djangoproject.com/ticket/21719#comment:13 went downhill
quickly] — I think we should remove it from the public API.
--
Ticket URL: <https://code.djangoproject.com/ticket/24313>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by carljm):
* stage: Unreviewed => Accepted
Comment:
I agree. ISTM that since app-loading, Django shouldn't even need
`class_prepared` internally anymore - but I realize there are dragons in
that code.
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:1>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by carljm):
For reference, the only place Django internally uses `class_prepared` is
to call `do_pending_lookups` to handle unresolved relations to the just-
loaded model. It certainly seems that could/should be handled by the app
registry instead...
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:2>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Follow the link in the description for the story of the last time you sent
me down this rabbit hole :-)
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:3>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by AlexHill):
Replying to [comment:2 carljm]:
> For reference, the only place Django internally uses `class_prepared` is
to call `do_pending_lookups` to handle unresolved relations to the just-
loaded model. It certainly seems that could/should be handled by the app
registry instead...
Hi Carl, I've opened ticket #24215 and
[https://github.com/django/django/pull/3984/files written a patch] to do
just this - feedback welcome.
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:4>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
There's also a
[https://github.com/django/django/blob/27c839e0fce99254ad61322bb827a821f832e840/django/db/models/signals.py#L15-L18
usage in ModelSignal]. It's not immediately obvious to me how/if this can
be refactored to use the app registry instead.
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:5>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by timgraham):
The `class_prepared` usage mentioned in my previous comment is removed as
part of #26421 so it looks like this should be doable after that's merged.
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:6>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by AlexHill):
Yep `class_prepared` is no longer used anywhere in Django other than tests
AFAIK. Unless someone can outline a use that can't be implemented with
`AppConfig.ready()`, I agree with deprecating it because
* it's unlike other signals in that the trigger is generally expected to
happen only once per sender
* it's hard to make sure it's registered in the right place
Mezzanine uses `class_prepared` to add fields to models at boot time,
which sounds nasty but works surprisingly well. Until Django has a
sanctioned method of swapping models besides `auth.User`, we'll continue
to support this. I'll try to refactor it to use `AppConfig.ready()` and
see how that goes.
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:7>
Django
Reporter: aaugustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by AlexHill):
Alright, there is at least one gap in functionality: using
`class_prepared`, you can fiddle with models in ways that propagate to
their subclasses. In `AppConfig.ready()`, since all the `ModelBase`
inheritance magic has already happened, it's too late to do anything like
that.
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:8>
Django
Reporter: Aymeric Augustin | Owner: Gurpreet
Type: | Singh
Cleanup/optimization | Status: assigned
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Gurpreet Singh):
* owner: nobody => Gurpreet Singh
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:9>
Django
Reporter: Aymeric Augustin | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Gurpreet Singh):
* owner: Gurpreet Singh => (none)
* status: assigned => new
Comment:
Replying to [comment:8 Alex Hill]:
> Alright, there is at least one gap in functionality: using
`class_prepared`, you can fiddle with models in ways that propagate to
their subclasses. In `AppConfig.ready()`, since all the `ModelBase`
inheritance magic has already happened, it's too late to do anything like
that.
I shouldn't have chosen to work on this as my first ticket then it seems.
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:10>
Django
Reporter: Aymeric Augustin | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Core (Other) | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Michael Manfre):
Another use case for `class_prepared` is allowing an abstract Model to
define indexes that 1) require a name (e.g. has conditions) where the
inheriting class name is too long for the index 30 character limit, or 2)
should be inherited even if the subclass defines its own indexes.
--
Ticket URL: <https://code.djangoproject.com/ticket/24313#comment:9>