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

False "get_model" Errors #121

Closed
flixx opened this issue Oct 27, 2020 · 2 comments · Fixed by #123
Closed

False "get_model" Errors #121

flixx opened this issue Oct 27, 2020 · 2 comments · Fixed by #123
Assignees

Comments

@flixx
Copy link
Member

flixx commented Oct 27, 2020

Hello :)

we're just updated to the lastest version and now get some of those errors:

'forward': Could not find an 'apps.get_model("...", "model")' call. Importing the model directly is incorrect for data migrations.

The data-migration looks as follows:

// ...
def forward(apps, schema_editor):
    organization_model: td.Organization = apps.get_model("b3_organization.Organization")
// ...

As you see there, get_model is defined - however, looking at the regex that caused this error, it seems that it expects 2 arguments. However, according to Djangos documentation, get_model() can also be shartcuted with just one argument. The linter does not accept that.

For now, we'll workaround by ignoring affected historic migrations.

Other then that, this new feature looks very interesting and useful!

Felix

@flixx
Copy link
Member Author

flixx commented Oct 27, 2020

Also those here the linter seems to be a bit too strict:

def remove_business_day_templates_for_workstations(apps, schema_editor):
    business_day_template_model = apps.get_model("b3_mes", "BusinessDayTemplate")

    for day in business_day_template_model.objects.all().iterator():
        day.delete()

resultes in:

(b3_mes, 0036_auto_20191217_1108)... ERR
	'remove_business_day_templates_for_workstations': Could not find an 'apps.get_model("...", "model")' call. Importing the model directly is incorrect for data migrations.

Changing the variable name to BusinessDayTemplate gives a PEP8 error here ("Variable in function should be lowercase").

@David-Wobrock
Copy link
Collaborator

Hi @flixx ! I hope you're well in Berlin :)

Good catch! The data migrations are still a bit early stage and strict.
I think you should be able to disable this check for now by using --exclude-migration-tests DATA_MIGRATION_MODEL_IMPORT

I completely missed the shortcut by using only one param, I'll try to implement it in the next days/weeks when I find some time 👍

About the strict validation of the model name, it's indeed a bit harsh, however, it would make more sense to use the model name and have BusinessDayTemplate = apps.get_model("b3_mes", "BusinessDayTemplate") instead of business_day_template_model = apps.get_model("b3_mes", "BusinessDayTemplate"). It should be, IMHO, the privileged way of naming the result of the get_model
I'll try to implement this as a separate check, but only as a warning that can be disabled

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

Successfully merging a pull request may close this issue.

2 participants