Opened 3 years ago

Closed 2 years ago

#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 3 years ago by charettes

  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years 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 3 years ago by syphar

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

comment:4 Changed 3 years 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 3 years 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 3 years 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 3 years 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 3 years ago by syphar (previous) (diff)

comment:8 Changed 3 years 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 3 years 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 3 years 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 3 years 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 3 years ago by timo

  • Patch needs improvement set

comment:13 Changed 2 years 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 2 years 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 2 years 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