[Django] #21477: Rename pre/post_migrate argument db to using
Django
------------------------------------------------+------------------------
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
------------------------------------------------+------------------------
All the other model signals have argument 'using', but pre/post_migrate
have the same information (that is, connection's alias) in argument db.
--
Ticket URL: <https://code.djangoproject.com/ticket/21477>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by charettes):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:1>
Django
Reporter: akaariai | Owner:
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by syphar):
wanted to work on this.
I assume we have to follow the deprecation policy since we change the name
of the provided kwarg?
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:2>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by syphar):
* owner: => syphar
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:3>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by charettes):
@syphar, the deprecation policy doesn't apply here since those signals
we're never part of a public release; they are only present on the master
branch.
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:4>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by syphar):
Just a thought, then why did Andrew keep not used arguments and built the
new signals as alias to the old ones?
- see this
(https://github.com/django/django/commit/68e0a169c4f9fa7f8071e014b274fd59e970f9a3),
- deprecation here
https://github.com/django/django/blame/master/docs/internals/deprecation.txt#L167
It seems like the two kwargs `create_models` and `created_models` are only
kept to stay backwards compatible.
Or am I wrong and these arguments _are_ still important, even if you use
migrations? (so, not comparable to this ticket).
Thanks for looking into this.
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:5>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by loic84):
@charettes `pre/post_syncdb` which now alias to `pre/post_migrate` were
public though.
Instead of aliasing I would keep `pre/post_syncdb` as their own thing and
just let the `db` arg die along with them, that would also allow a
`DeprecationWarning` which is currently missing.
I think it's a win-win, we do the cleanup and we offer a better
deprecation path than currently.
PR coming soon.
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:6>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by syphar):
@loic84
i have a WIP branch here
- https://github.com/syphar/django/compare/master...ticket_21477
feel free to use it.
Won't be able to finish it today.
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:7>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by loic84):
* cc: loic@… (added)
Comment:
@syphar I somehow missed your earlier comment, I guess we were writing at
the same time, ccing to the ticket so I get a heads up next time.
There is indeed an issue with `create_models` and `created_models`. They
are used internally for things like
https://github.com/django/django/blob/master/django/contrib/sites/management.py#L15,
I'm not sure what will be the replacement. Anyhow that's probably outside
the scope of this ticket.
I also have a very similar branch with the deprecation warnings:
https://github.com/loic/django/compare/syncdb_signals.
You claimed the ticket first, so I'll defer to you; feel free to ping me
if you'd rather me to take it from there.
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:8>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by syphar):
* has_patch: 0 => 1
Comment:
I've tried to put the work by loic84 and me together. Result is this here:
https://github.com/django/django/pull/1983
I also added tests for the DeprecationWarning
I'm not entirely sure if
- the documentation should be done differently (no experience here on my
side)
- the DeprecatedSignal is a thing we will need more often and if we should
make it more general and put into
https://github.com/django/django/blob/master/django/utils/deprecation.py
Any thoughts on this?
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:9>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by andrewgodwin):
I like this approach - we get a DeprecationWarning for the old signals out
of it too, which is a bonus.
I vote +1 on moving DeprecatedSignal somewhere more general (deprecation
is probably good, or possibly django.dispatch). Docs look good apart from
one small typo that I noted on the PR.
Fix those and make the PR mergeable and I'll pull it in.
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:10>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Comment (by loic84):
The `DeprecationWarning` is hardcoded, so if we move `DeprecatedSignal`
with the goal of making it generic it should probably take the warning
level as an `__init__` argument.
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:11>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by timo):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:12>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by loic84):
* severity: Normal => Release blocker
Comment:
These patches also address the deprecation of the pre/post_syncdb signals
which IMO should be part of Django 1.7.
Bumping the severity to Release Blocker as a result, please revert if you
think otherwise.
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:13>
Django
Reporter: akaariai | Owner: syphar
Type: Cleanup/optimization | Status: assigned
Severity: Release blocker | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Comment (by aaugustin):
I've restored pre/post_syncdb and made pre/post_migrate a different set of
signals since this discussion.
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:14>
Django
Reporter: akaariai | Owner: syphar
Component: Migrations | Version: master
Severity: Release blocker | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 1 | UI/UX: 0
Changes (by Aymeric Augustin <aymeric.augustin@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"d562527a160f420c6af0d2736ad4e6c87b0d2ef1"]:
{{{
#!CommitTicketReference repository=""
revision="d562527a160f420c6af0d2736ad4e6c87b0d2ef1"
Fixed #21477 -- Renamed db to using in pre/post_migrate signals.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/21477#comment:15>