#21477 closed Cleanup/optimization (fixed)

Rename pre/post_migrate argument db to using

Reported by: akaariai Owned by: syphar
Component: Migrations Version: master
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 Changed 18 months ago by charettes

  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 18 months ago 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?

comment:3 Changed 18 months ago by syphar

  • Owner set to syphar
  • Status changed from new to assigned

comment:4 Changed 18 months ago 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.

comment:5 Changed 18 months ago by syphar

Just a thought, then why did Andrew keep not used arguments and built the new signals as alias to the old ones?

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 Changed 18 months ago 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.

comment:7 Changed 18 months ago by syphar

@loic84
i have a WIP branch here

feel free to use it.

Won't be able to finish it today.

No DeprecationWarning atm, but I like the idea.

Last edited 18 months ago by syphar (previous) (diff)

comment:8 Changed 18 months ago by loic84

  • Cc loic@… 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 Changed 18 months ago by syphar

  • 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

Any thoughts on this?

comment:10 Changed 18 months ago 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.

comment:11 Changed 18 months ago 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.

comment:12 Changed 18 months ago by timo

  • Patch needs improvement set

comment:13 Changed 17 months ago by loic84

  • Severity changed from Normal to 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 Changed 17 months ago by aaugustin

I've restored pre/post_syncdb and made pre/post_migrate a different set of signals since this discussion.

comment:15 Changed 17 months ago by Aymeric Augustin <aymeric.augustin@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In d562527a160f420c6af0d2736ad4e6c87b0d2ef1:

Fixed #21477 -- Renamed db to using in pre/post_migrate signals.

Note: See TracTickets for help on using tickets.
Back to Top