#34568 closed Bug (fixed)
makemigrations --update should respect the --name option.
Reported by: | David Sanders | Owned by: | Mariusz Felisiak |
---|---|---|---|
Component: | Database layer (models, ORM) | Version: | 4.2 |
Severity: | Release blocker | Keywords: | |
Cc: | Simon Charette, David Wobrock | Triage Stage: | Accepted |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
This may be intentional behaviour but the docs don't mention this so creating a ticket to update docs or correct behaviour:
If you create a migration with a custom name:
$ ./manage.py makemigrations --name foo Migrations for 'update_rename': update_rename/migrations/0001_foo.py - Create model Foo
then running --update
will change the name "foo" to the autogenerated one based on the operations:
$ ./manage.py makemigrations --update Migrations for 'update_rename': update_rename/migrations/0001_initial.py - Create model Foo Deleted update_rename/migrations/0001_foo.py
My opinion is that it shouldn't as it violates the principle of least astonishment even though the --name
argument wasn't supplied.
EDIT:
This is my first time using --update and here are a few other observations which could indicate that it requires broader discussion:
- It doesn't utilise the --name argument so it's impossible to customise the name during --update
- It'd be nice to provide --no-optimize option to --update, here's my use-case: 3-step non-null field addition. After doing nullable step 1, elidable data migration step 2, I want to merge the step 3 non-null update into the migration but --update optimizes this into a single step.
Perhaps --update requires a rethink?
Change History (16)
comment:2 by , 19 months ago
Description: | modified (diff) |
---|
comment:3 by , 19 months ago
Cc: | added |
---|
I think you're expecting too much from --update
was intended to be an option for updating recently developed changes and don't do anything fancier. Please check the discussion in βPR.
My opinion is that it shouldn't as it violates the principle of least astonishment even though the --name argument wasn't supplied.
We could document this behavior, but IMO it shouldn't be changed.
It'd be nice to provide --no-optimize option to --update, here's my use-case: 3-step non-null field addition. After doing nullable step 1, elidable data migration step 2, I want to merge the step 3 non-null update into the migration but --update optimizes this into a single step.
It's probably worth adding π€
comment:4 by , 19 months ago
@felix read through that PR, I'm assuming your point about ending up with a name that doesn't match the operations is referring to not being able to tell the difference between a custom name and an auto gen'd name. Here's my response:
- It's a fair point. Trying to determine custom names is too complex and shouldn't be done.
- In my anecdotal experience we (myself and colleagues) rarely use the auto gen'd name. An option to
--keep-name
sounds nice to me with the default to *not keep*. - If not, then at the very least supplying
--name
should raise an exception or honour the name. It definitely shouldn't do *nothing*. My preference is to honour--name
π
Can we get Simon's take as well since he was part of the thread?
I still think something should change because it's not even fancyβ¦Β I'd say that turning off optimisations is harder than this.
comment:5 by , 19 months ago
Also just now thought of another option:
- An option to print migrations to stdout
--stdout
(or whatever) then I can redirect the content to my latest migration and edit as necessary π
comment:6 by , 19 months ago
Cc: | added |
---|
comment:7 by , 19 months ago
After reading the referenced PR and the comments in this ticket, I agree with David that ignoring --name
feels like a bug, and I think it's worth fixing. I believe the fix would be as simple as (plus tests):
-
django/core/management/commands/makemigrations.py
a b class Command(BaseCommand): 316 316 ) 317 317 # Update name. 318 318 previous_migration_path = MigrationWriter(leaf_migration).path 319 suggested_name = (319 suggested_name = self.migration_name or ( 320 320 leaf_migration.name[:4] + "_" + leaf_migration.suggest_name() 321 321 ) 322 322 if leaf_migration.name == suggested_name:
Regarding optimization, I would have taken a different approach in the original implementation: do not optimize the updated migration, leave that decision to the caller, they can later call optimizemigration
. Is it too late for considering that approach?
Lastly, I would not go the route of allowing to keep the current name, the caller can achieve that by passing --name
which is already a documented and valid option for the command.
follow-up: 9 comment:8 by , 19 months ago
I also think that --name
should not be ignored when --update
is used but updating the name of the updated migration to follow the usual operation fragment stitching otherwise seems like a expected default.
Whether or not we we should add an option not to perform optimization or disable operation fragment usage for migration name entirely seems like distinct feature requests.
comment:9 by , 19 months ago
Replying to Simon Charette:
Whether or not we we should add an option not to perform optimization or disable operation fragment usage for migration name entirely seems like distinct feature requests.
Yep, though before I create a ticket for that, I might just create a PoC to see whether it helps with the use case I described above.
comment:10 by , 19 months ago
Severity: | Normal β Release blocker |
---|---|
Summary: | makemigrations --update changes the name of custom migration name β makemigrations --update should respect the --name option. |
Triage Stage: | Unreviewed β Accepted |
Type: | Uncategorized β Bug |
I'm happy to treat this as a bug in the new feature. Opt-out optimization is a separate feature request.
follow-up: 16 comment:11 by , 18 months ago
One more thing:
--update
is a destructive operation βΒ if you had any customised migration operations or code in your latest migration this will be permanently deleted if you run update *without any warning*.
I'd like to suggest that *at least* one of the following happen:
-
--update
does a confirmation eg "<app>/migrations/0009_last_migration.py will be replaced. Proceed? y/N". Along with this we provide a--no-input
. Both of these are consistent with other commands. - we document that it destroys your last migration without warning
My preference is 1. because, to paraphrase FunkyBob, the purpose of any framework is to manage the risky and the tedious.
We should at the very least do 2. if it's decided 1. is a no-go.
This also sounds like another ticket.
comment:12 by , 18 months ago
Owner: | changed from | to
---|---|
Status: | new β assigned |
comment:16 by , 18 months ago
Replying to David Sanders:
One more thing:
--update
is a destructive operation βΒ if you had any customised migration operations or code in your latest migration this will be permanently deleted if you run update *without any warning*.
I'd like to suggest that *at least* one of the following happen:
--update
does a confirmation eg "<app>/migrations/0009_last_migration.py will be replaced. Proceed? y/N". Along with this we provide a--no-input
. Both of these are consistent with other commands.- we document that it destroys your last migration without warning
My preference is 1. because, to paraphrase FunkyBob, the purpose of any framework is to manage the risky and the tedious.
We should at the very least do 2. if it's decided 1. is a no-go.
This also sounds like another ticket.
I agree this should be another ticket, and I also agree overwriting without any warning feels like an antipattern. I would definitely go with (1).
Having said the above, are you positive overwrites are happening? If so, could you please provide the sequence of commands that would trigger that situation in the new ticket? Thanks!
<moved to description>