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

Add Django 4.2 support on the write_migration_files method #268

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

faradayyg
Copy link

@faradayyg faradayyg commented Sep 29, 2023

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 faradayyg marked this pull request as ready for review September 29, 2023 07:08
@faradayyg
Copy link
Author

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
Copy link
Collaborator

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
Copy link

codecov-commenter commented Oct 28, 2023

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%.

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

Additional details and impacted files
@@            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 Coverage Δ
...ation_linter/management/commands/makemigrations.py 92.42% <66.66%> (-4.19%) ⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@David-Wobrock
Copy link
Collaborator

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
Copy link
Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants