Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#25453 closed Cleanup/optimization (fixed)

makemigrations suggestion seems to go against best practices

Reported by: Benjamin Wohlwend Owned by: nobody
Component: Migrations Version: 1.8
Severity: Normal Keywords:
Cc: piquadrat@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When creating a migration that makes a formerly nullable field non-nullable, makemigrations has these suggestions:

You are trying to change the nullable field 'foo' on bar to non-nullable without a default; we can't do that (the database needs something to populate existing rows).
Please select a fix:
 1) Provide a one-off default now (will be set on all existing rows)
 2) Ignore for now, and let me handle existing rows with NULL myself (e.g. adding a RunPython or RunSQL operation in the new migration file before the AlterField operation)
 3) Quit, and let me add a default in models.py

The second option seems to suggest to add the RunPython operation in the same migration file as the AlterField operation, which is against best practices of splitting schema- and data-migrations.

This seems to be somewhat related to #24630, which updated the documentation to show how to split up the migration (albeit for a slightly different use case). I'm not sure how the output from makemigrations could be worded better, but it could e.g. link to the updated docs.

Change History (8)

comment:1 Changed 6 years ago by Benjamin Wohlwend

Cc: piquadrat@… added

comment:2 Changed 6 years ago by Tim Graham

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

Could it say, "in a migration file before the AlterField operation"? I prefer avoiding documentation links in the code if possible since they can be difficult to maintain if the docs are restructured.

comment:3 Changed 6 years ago by Benjamin Wohlwend

It's a bit of a temporal issue. At the point where it asks you, you should already have created the data migration. How about

Ignore for now, and let me handle existing rows with NULL myself (e.g. because you added a RunPython or RunSQL operation to handle NULL values in a previous data migration)

It's a bit longer, but it mentions "data migration", which is pretty google-able for people that are a bit lost at this point.

comment:4 Changed 6 years ago by Tim Graham

Works for me.

comment:5 Changed 6 years ago by Benjamin Wohlwend

Has patch: set

comment:6 Changed 6 years ago by Tim Graham

Triage Stage: AcceptedReady for checkin

comment:7 Changed 6 years ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In de314231:

Fixed #25453 -- Reworded makemigration's ask_not_null_alteration suggestion.

comment:8 Changed 6 years ago by Tim Graham <timograham@…>

In 67896c8:

[1.8.x] Fixed #25453 -- Reworded makemigration's ask_not_null_alteration suggestion.

Backport of de31423130912a012513aad93ec805f8e08a0d5e from master

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