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

Rule N806 and Django #7675

Closed
aqeelat opened this issue Sep 27, 2023 · 7 comments · Fixed by #8917
Closed

Rule N806 and Django #7675

aqeelat opened this issue Sep 27, 2023 · 7 comments · Fixed by #8917
Assignees
Labels
accepted Ready for implementation rule Implementing or modifying a lint rule

Comments

@aqeelat
Copy link

aqeelat commented Sep 27, 2023

In django, you can access a model by importing it or by getting it from the registry (i.e. calling apps.get_model). The convention is to name the variable in title case to match the model's name.
Example from the docs: https://docs.djangoproject.com/en/4.2/ref/applications/#django.apps.AppConfig.ready

However, this clashes with rule N806.

Is there a way to make this work?

@aqeelat
Copy link
Author

aqeelat commented Sep 27, 2023

I think this could a setting for N806 that allows(\w*) = .*\.get_model\(.*,"(\w*)"\) if $1 and $2 match.

@charliermarsh
Copy link
Member

I'm open to supporting this, although I think it should just be enabled and not behind a setting. I think we need to support both apps.get_model("core", "Activation") and (e.g.) the MyModel = self.get_model("MyModel") form.

@charliermarsh charliermarsh added rule Implementing or modifying a lint rule accepted Ready for implementation labels Sep 29, 2023
@q0w
Copy link

q0w commented Oct 4, 2023

Also import_string

@charliermarsh charliermarsh self-assigned this Nov 30, 2023
charliermarsh added a commit that referenced this issue Nov 30, 2023
…n-function` (`N806`) (#8917)

## Summary

Allows assignments of the form, e.g., `Attachment =
apps.get_model("zerver", "Attachment")`, for better compatibility with
Django.

Closes #7675.

## Test Plan

`cargo test`
@tylerlaprade
Copy link
Contributor

This fix is great! We had previously turned this rule off in our migrations/ folder for exactly this reason. However, if we remove that override, we have a few cases where we need to dynamically set the model, so $1 and $2 don't match:

image image image

@charliermarsh
Copy link
Member

I suppose we could extend this to allow any apps.get_model-like assignments, and not limit it to matches on the left- and right-hand side...

@tylerlaprade
Copy link
Contributor

tylerlaprade commented Dec 9, 2023

After going through and renaming our violations, I don't feel as strongly that the rule should change. Perhaps the restriction should only be relaxed when the right-hand side is a variable? I know this would still cause my third screenshot to be broken, but I think there's more value in ensuring you didn't typo your model name than there is in allowing this one-off rare case.

@charliermarsh
Copy link
Member

That seems reasonable to me. I added it in: #9065.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepted Ready for implementation rule Implementing or modifying a lint rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants