Skip to content
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

Use MigrationLoader to order migrations #208

Merged
merged 16 commits into from
Jan 3, 2023
Merged

Conversation

q0w
Copy link
Contributor

@q0w q0w commented Oct 17, 2022

Fix #161

What do you think @adamchainz? It also seems using MigrationLoader needs proper fake migrations (with Migration class, not just empty files)

@adamchainz adamchainz marked this pull request as ready for review October 17, 2022 10:28
Comment on lines 114 to 119
plan = []
for node in nodes:
for migration in loader.graph.forwards_plan(node):
if migration not in plan:
plan.append(migration)
return plan
Copy link
Contributor Author

@q0w q0w Oct 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd feel more comfortable calling the method in Django, even if it is private.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, i will use typing.cast to avoid Any

@adamchainz
Copy link
Owner

Looks like a good start. (Please don't create draft PR's if you want them looked at, "draft" normally means "don't look at this".)

In the issue I noted we'd need to call detect_conflicts().

The test run failures are due to lack of Migration which you said.

Will you keep working on this?

@q0w
Copy link
Contributor Author

q0w commented Oct 17, 2022

Yea, i am going to rewrite tests.
Where should it call detect_conflicts and how it should be handled?

@q0w q0w force-pushed the issue-161 branch 2 times, most recently from 8b83882 to e6a5891 Compare October 17, 2022 14:50
@q0w q0w requested a review from adamchainz October 17, 2022 14:51
@q0w
Copy link
Contributor Author

q0w commented Oct 24, 2022

I've added dlm.E000 as Error for conflicts in the migration graph.

Copy link
Owner

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just getting round to this now after a busy end of 2022...

Please, whenever you open an open source PR, update the documentation and add a changelog note.

tests/utils.py Outdated Show resolved Hide resolved
src/django_linear_migrations/apps.py Outdated Show resolved Hide resolved
src/django_linear_migrations/apps.py Outdated Show resolved Hide resolved
src/django_linear_migrations/apps.py Outdated Show resolved Hide resolved
src/django_linear_migrations/apps.py Outdated Show resolved Hide resolved
@q0w
Copy link
Contributor Author

q0w commented Jan 3, 2023

What do you think about adding a new flag to provide the db name instead of using always DB_DEFAULT_ALIAS in MigrationLoader?

@adamchainz
Copy link
Owner

It seems we can use None for connection and skip loading migration state from the database entirely: https://github.com/django/django/blob/898f0aa44fe63c20d7f4125672b8eb800a578ce5/django/db/migrations/loader.py#L230-L235

@adamchainz adamchainz merged commit b945dc0 into adamchainz:main Jan 3, 2023
@adamchainz
Copy link
Owner

Released in 2.6.0, thanks!

@q0w q0w deleted the issue-161 branch January 3, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants