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

QF-2700 Run makemigrations --check right after building #675

Merged
merged 3 commits into from
Jun 5, 2023

Conversation

why-not-try-calmer
Copy link
Contributor

@why-not-try-calmer why-not-try-calmer commented Jun 1, 2023

What looked like adding a single line of code turned into me arm-wrestling the Django migration writer.

The issue is basically that the migration writer doesn't know how to serialize bound functions -- functions parameterized with an implicit reference -- and that static methods seem to be considered as bound functions.

I went for the "brutal" way of solving this: i.e. extracting the static method to a top-level function. There are other ways. Please make a commit suggestion if you are not happy with the way I went for.

@duke-nyuki
Copy link
Collaborator

Task linked: QF-2700 Run migrations check in CI

@why-not-try-calmer why-not-try-calmer changed the title makemigrations --check QF-2700 Run makemigrations --check right after building Jun 1, 2023
@why-not-try-calmer why-not-try-calmer added the chore Maintanance, clean-up and other not fancy improvements. label Jun 1, 2023
@m-kuhn
Copy link
Member

m-kuhn commented Jun 3, 2023

Thank you, I am not able to comment on the arm-wrestling approach, but I believe the libqfieldsync changes are not needed in here?

@why-not-try-calmer
Copy link
Contributor Author

why-not-try-calmer commented Jun 3, 2023

Thank you, I am not able to comment on the arm-wrestling approach, but I believe the libqfieldsync changes are not needed in here?

By default my editor completely hide changes to submodules. I had fixed this but a fresh reset brought the defaults back. Sorry

@m-kuhn
Copy link
Member

m-kuhn commented Jun 3, 2023

No worries, I think we should change that to a pypi package instead.

Copy link
Collaborator

@suricactus suricactus left a comment

Choose a reason for hiding this comment

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

LGTM

Interesting that the code had to be refactored like this, thanks for winning the wrestling contest!

I think it opens the discussion to retire the Geodb module once again.

@why-not-try-calmer why-not-try-calmer merged commit ed9a043 into master Jun 5, 2023
4 checks passed
@why-not-try-calmer why-not-try-calmer deleted the QF-2700_check_makemigrations branch June 5, 2023 07:36
why-not-try-calmer added a commit that referenced this pull request Jun 5, 2023
* Removed libqfieldsync changes

* fixed by removing staticmethod

* Removed libqfieldsync changes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Maintanance, clean-up and other not fancy improvements.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants