Opened 11 years ago
Closed 11 years ago
#21477 closed Cleanup/optimization (fixed)
Rename pre/post_migrate argument db to using
Reported by: | Anssi Kääriäinen | Owned by: | Denis Cornehl |
---|---|---|---|
Component: | Migrations | Version: | dev |
Severity: | Release blocker | Keywords: | |
Cc: | loic@… | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | yes | UI/UX: | no |
Description
All the other model signals have argument 'using', but pre/post_migrate have the same information (that is, connection's alias) in argument db.
Change History (15)
comment:1 by , 11 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 11 years ago
comment:3 by , 11 years ago
Owner: | set to |
---|---|
Status: | new → assigned |
comment:4 by , 11 years ago
@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.
comment:5 by , 11 years ago
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.
comment:6 by , 11 years ago
@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.
comment:7 by , 11 years ago
@loic84
i have a WIP branch here
feel free to use it.
Won't be able to finish it today.
comment:8 by , 11 years ago
Cc: | added |
---|
@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.
comment:9 by , 11 years ago
Has patch: | set |
---|
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?
comment:10 by , 11 years ago
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.
comment:11 by , 11 years ago
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.
comment:12 by , 11 years ago
Patch needs improvement: | set |
---|
comment:13 by , 11 years ago
Severity: | Normal → Release blocker |
---|
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.
comment:14 by , 11 years ago
I've restored pre/post_syncdb and made pre/post_migrate a different set of signals since this discussion.
comment:15 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
wanted to work on this.
I assume we have to follow the deprecation policy since we change the name of the provided kwarg?