New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed #26429 -- Placed a timestamp into generated merge migration names #6485

Merged
merged 3 commits into from May 11, 2016

Conversation

Projects
None yet
4 participants
@rtpg
Contributor

rtpg commented Apr 21, 2016

This sets a timestamp to the generated merge migration names, much in the same fashion as other migrations. This is to help reduce the possibility of a naming conflict (especially after squashing migrations).

This is to fix the issues mentioned in #26469, where the combination of merge migration names and squashing can cause a migration to end up never running due to a name conflict.

@rtpg rtpg changed the title from Place a timestamp into merge migration names to Fix #26469: Place a timestamp into generated merge migration names Apr 21, 2016

@charettes

This comment has been minimized.

Show comment
Hide comment
@charettes

charettes Apr 23, 2016

Member

You should be able to fix the failing tests by mocking datetime.datetime.now() to return a fixed date and adjust the expected filenames accordingly.

Member

charettes commented Apr 23, 2016

You should be able to fix the failing tests by mocking datetime.datetime.now() to return a fixed date and adjust the expected filenames accordingly.

@charettes charettes changed the title from Fix #26469: Place a timestamp into generated merge migration names to Fixed #26429 -- Placed a timestamp into generated merge migration names Apr 23, 2016

@MarkusH

This comment has been minimized.

Show comment
Hide comment
@MarkusH

MarkusH Apr 23, 2016

Member

I'm wondering if it may make sense to move the file name generation onto the Migration class (django.db.migrations.migration.Migration). The MigrationAutodetector has a method suggest_name() that already handles some cases. I feel that would be a more appropriate way to solve the issue.

Member

MarkusH commented Apr 23, 2016

I'm wondering if it may make sense to move the file name generation onto the Migration class (django.db.migrations.migration.Migration). The MigrationAutodetector has a method suggest_name() that already handles some cases. I feel that would be a more appropriate way to solve the issue.

@rtpg

This comment has been minimized.

Show comment
Hide comment
@rtpg

rtpg Apr 25, 2016

Contributor

I had tried another way of fixing this branch (by adding support for --name in the makemigrations command for merge migrations), but looking at suggest_name that seems like the "proper" solution. Plus it helps reduce the amount of command-only code. I'll try moving the name generation code there.

I felt a bit uneasy at patching datetime.datetime.now but thinking about it now, that would be a good way of testing that the default naming scheme works as expected, I'll go with that as well.

Contributor

rtpg commented Apr 25, 2016

I had tried another way of fixing this branch (by adding support for --name in the makemigrations command for merge migrations), but looking at suggest_name that seems like the "proper" solution. Plus it helps reduce the amount of command-only code. I'll try moving the name generation code there.

I felt a bit uneasy at patching datetime.datetime.now but thinking about it now, that would be a good way of testing that the default naming scheme works as expected, I'll go with that as well.

@rtpg

This comment has been minimized.

Show comment
Hide comment
@rtpg

rtpg Apr 25, 2016

Contributor

I looked into integrating the merge migration naming behaviour into the MigrationAutodetector class. It turns out that the autodetector is naming the files, but is mainly being used in the standard migration generator (the one that generates changes with "operations"). Merge migrations aren't really an operation, but just a declaration of dependencies, so it's all being done in a separate ad-hoc area.

The merge migration generation code is actually all within the makemigrations command under handle_merge, and shares very little with the code path that ends up using the MigrationAutodetector class. This is probably not too great; you have to run the command to test the merge migration generator logic. It would be worthwhile to pull out the merge migration handling code into its own module, though I don't know what the Django policy is on "refactor code without any behaviour changes associated".

Anyways, a long way of saying that I kept the initial way of generating the name because it keeps the diff small without being worse than the status quo. I also added a test that checks that the default naming scheme works properly through a timezone patch (switched from using datetime because of ease-of-mocking)

Contributor

rtpg commented Apr 25, 2016

I looked into integrating the merge migration naming behaviour into the MigrationAutodetector class. It turns out that the autodetector is naming the files, but is mainly being used in the standard migration generator (the one that generates changes with "operations"). Merge migrations aren't really an operation, but just a declaration of dependencies, so it's all being done in a separate ad-hoc area.

The merge migration generation code is actually all within the makemigrations command under handle_merge, and shares very little with the code path that ends up using the MigrationAutodetector class. This is probably not too great; you have to run the command to test the merge migration generator logic. It would be worthwhile to pull out the merge migration handling code into its own module, though I don't know what the Django policy is on "refactor code without any behaviour changes associated".

Anyways, a long way of saying that I kept the initial way of generating the name because it keeps the diff small without being worse than the status quo. I also added a test that checks that the default naming scheme works properly through a timezone patch (switched from using datetime because of ease-of-mocking)

@rtpg

This comment has been minimized.

Show comment
Hide comment
@rtpg

rtpg Apr 27, 2016

Contributor

Should I rebase this set of commits? Would doing so mess up the pull request (not too sure about how well Github's PR system plays with git rebasing)

Contributor

rtpg commented Apr 27, 2016

Should I rebase this set of commits? Would doing so mess up the pull request (not too sure about how well Github's PR system plays with git rebasing)

Show outdated Hide outdated tests/migrations/test_commands.py Outdated
Show outdated Hide outdated tests/migrations/test_commands.py Outdated
Show outdated Hide outdated tests/migrations/test_commands.py Outdated
Place a timestamp into merge migration names by default
 This sets a timestamp to the default names for generated merge migrations, much in the same fashion as other migrations. This is to help reduce the possibility of a naming conflict (especially after squashing migrations).
@rtpg

This comment has been minimized.

Show comment
Hide comment
@rtpg

rtpg May 8, 2016

Contributor

Sorry for the wait!

I cleaned up the code according to the suggestions and rebased it down to one commit, I believe this can be merged now.

Contributor

rtpg commented May 8, 2016

Sorry for the wait!

I cleaned up the code according to the suggestions and rebased it down to one commit, I believe this can be merged now.

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham May 10, 2016

Member

Looks mostly good. How about these edits? rtpg#1

Member

timgraham commented May 10, 2016

Looks mostly good. How about these edits? rtpg#1

@rtpg

This comment has been minimized.

Show comment
Hide comment
@rtpg

rtpg May 11, 2016

Contributor

Those look good, merging it in.

Contributor

rtpg commented May 11, 2016

Those look good, merging it in.

@timgraham timgraham merged commit 8f6a1a1 into django:master May 11, 2016

6 checks passed

default Build finished.
Details
docs Build finished.
Details
flake8 Build finished.
Details
isort Build finished.
Details
javascript Build finished.
Details
windows Build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment