3YOURMIND / django-migration-linter

:rocket: Detect backward incompatible migrations for your django project
https://pypi.python.org/pypi/django-migration-linter/
Apache License 2.0
530 stars 58 forks source link

Add Django 4.2 support on the `write_migration_files` method #268

Closed faradayyg closed 2 months ago

faradayyg commented 1 year ago

Django 4.2 introduced a new optional argument to the write_migration_files method of the Command class.

While the current implementation works, it makes sense to modify to code so that the function parameters match, depending on what Django version is installed.

faradayyg commented 1 year ago

Hello @David-Wobrock I'm not sure if you have seen this PR yet, but please take a look at this and let me know what you think.

David-Wobrock commented 1 year ago

Hey @faradayyg

Thanks for the PR and the ping.

I'll try to take some time soon to give some love again to the linter :)

codecov-commenter commented 1 year ago

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (117967a) 94.10% compared to head (ed7faa3) 93.97%.

:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #268 +/- ## ========================================== - Coverage 94.10% 93.97% -0.13% ========================================== Files 81 81 Lines 2034 2041 +7 ========================================== + Hits 1914 1918 +4 - Misses 120 123 +3 ``` | [Files](https://app.codecov.io/gh/3YOURMIND/django-migration-linter/pull/268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) | Coverage Δ | | |---|---|---| | [...ation\_linter/management/commands/makemigrations.py](https://app.codecov.io/gh/3YOURMIND/django-migration-linter/pull/268?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None#diff-ZGphbmdvX21pZ3JhdGlvbl9saW50ZXIvbWFuYWdlbWVudC9jb21tYW5kcy9tYWtlbWlncmF0aW9ucy5weQ==) | `92.42% <66.66%> (-4.19%)` | :arrow_down: |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

David-Wobrock commented 1 year ago

Hey @faradayyg

Thanks for the PR and interesting in the project!

About the PR

First, I'll need you to rebase the PR against main, which had a few structure changes - and second, to fix CI (linting mainly). Ideally, we should also add unit tests :)

On the idea of the PR

The addition of the optional update_previous_migration_paths parameter is linked to the new Django makemigrations option: update - which will update the latest migration instead of creating a new one (I know the person who added this option 🙄)

So what should the linter in this situation? Because potentially, the linter will not allow creating a new migration if the linting fails. However, we do this by generating the SQL on an existing migration file, meaning that if want to reject a new migration, we actually delete the file. But if we are updating the latest migration, but the changes are not backward compatible, does that mean that we are deleting the last migration? Or just undo the last changes? But the latter is more difficult with our current design (but possible I think).

So let's add tests about the --update option, and try to define how we want to support this new feature :)

faradayyg commented 1 year ago

Thanks for the detailed response @David-Wobrock

I'll take a look and fix the issues.

I'll also try to make a decision on the update_previous_migration_paths issue.

faradayyg commented 2 months ago

One year later, I have not been able to get to this. I will be closing this PR. Thank you!