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

Fix for django >= 4.0 makemigrations #184

Merged
merged 2 commits into from
May 16, 2024
Merged

Conversation

alcad
Copy link
Contributor

@alcad alcad commented Jul 11, 2023

It's necessary to specify the default_auto_field param in apps.py to inhibit generation of migrations in package

It's necessary to specify the default_auto_field param in apps.py to
inhibit generation of migrations in package
@alcad
Copy link
Contributor Author

alcad commented Jul 11, 2023

This PR refers to yet well documented issue #182

@ticosax
Copy link
Member

ticosax commented Jul 12, 2023

Will be solved only through #133 , I'm afraid.

@alcad
Copy link
Contributor Author

alcad commented Jul 12, 2023

Hi @ticosax You don't need an abstract model and cannot force user to extend your.

Imagine this use case:
i create a new migration for fsm_log (even extending and mantaining in my project); all my new models and migrations depending on it will evolve during time. If you tomorrow you release a new migration in django_fsm_log altering AutoField bringing it to BigAutoField i will have a conflict in my db because cannot apply your migration; probably i can revert my migration, but what if my id cannot be reverted because i have a lot o records? I can revert fake my migration and apply fake your but you understand that is very boring.

The alternative is to migrate your model to BigAutoField and persist in apps.py so the migration is your and mantained in the package meanwile #133 will be well defined.

Let me know what you think please

Regards

@ticosax
Copy link
Member

ticosax commented Jul 12, 2023

The changes brought by this PR, were already attempted and discussed.

#132
#138

I don't know if you already acknowledged them ?

@alcad
Copy link
Contributor Author

alcad commented Jul 13, 2023

Hi @ticosax i read all before pull request.

The reason for my PR is that the new django >= 4.0 default for new projects is BigAutoField so every new project started with this package create a migrations outside te boundaries of the project and request manual interventions to recover problematic situations like deploy on a server or sharing with colleagues (you cannot migrate nothing or generate migrations because of missing migrations outside project).

Can we al least imagine a fix for this specific use case?

Best regards

@ticosax
Copy link
Member

ticosax commented Jul 13, 2023

Can we al least imagine a fix for this specific use case?

To my knowledge, the better, known today, approach to solve this issue, is captured in #133.
Unfortunately it's not a easy & fast solution. Given we don't dedicate much time to it.
I started a PoC in #176. It's far to be finished, but it might be good enough, to get the ball rolling and push it to the finish line.

@logginst
Copy link

I'd also advocate for this change, over moving to AbstractModel. We ended up going with a monkey patch of the config class as a solution in the meantime:

class CustomDjangoFSMLogConfig(DjangoFSMLogAppConfig):
    """
    This Config class is meant to replace the AppConfig of the `django_fsm_log`
    third-party package. Since this app has not yet been updated to be compatible
    with the `default_auto_field` config setting introduced in Django 3.2, we
    get a warning related to uncreated migrations. This patch fixes this issue.
    """

    default_auto_field = "django.db.models.AutoField"

For what it's worth, this was also the proposed and accepted fix in some other third-party libraries that encountered the same issue. For example, https://github.com/jazzband/django-invitations/pull/211/files

@lorenzomorandini
Copy link
Contributor

Given the current state of the project I agree with merging the PR. Moving to an abstract model doesn't seem worth it.

@lorenzomorandini
Copy link
Contributor

@ticosax could we merge?

@Vyko
Copy link

Vyko commented Feb 22, 2024

Hi @ticosax,
Generally speaking, thanks a lot for your work on this project !
Could we see this merged as it's an issue occurring since 2021 ? 🙏

@lorenzomorandini lorenzomorandini merged commit 1e701c6 into jazzband:master May 16, 2024
7 checks passed
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.

5 participants