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

Tests: Add test case for settings import cycle #2098

Merged
merged 2 commits into from
May 1, 2024

Conversation

intgr
Copy link
Collaborator

@intgr intgr commented Apr 30, 2024

Add a testcase that demonstrates the 'Import cycle from Django settings module prevents type inference' error and ensures it doesn't regress.

Related:

Copy link
Member

@flaeppe flaeppe left a comment

Choose a reason for hiding this comment

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

Nice that we now get coverage for this case. But I'm a bit baffled, since I would have liked mypy to figure this out instead of us.

If I set up a similar case with a cyclic import(see below). Mypy is able to figure out all types without any issues. I might be wrong here but what I'm saying is that it could be an improvement if we were able to tell mypy to behave like my case below. I don't know how or if it's even possible, since it probably depends on at what stage the plugin gets involved, but since mypy is able to solve it perhaps there's a possibility for the plugin to poke mypy in the direction to behave like it does to figure out the below case.

Our plugin being the one emitting the error isn't any serious issue really, since adding explicit types is a solution, decent as well. And an alternate approach might even be larger and more complex. I just wanted to lift the idea here.

Here's the cyclic case I was thinking about:

# a.py
from b import function_returning_int
VAL = function_returning_int()
reveal_type(VAL)
# b.py
import a

def test() -> None:
    reveal_type(a.VAL)

def function_returning_int() -> int:
    return 42

@intgr
Copy link
Collaborator Author

intgr commented May 1, 2024

Agreed, it would be nice to solve this in plug-in code and avoid the circular import error entirely. But given that the work-around isn't complicated, it's not a high priority from my PoV.

@intgr intgr merged commit a10f8aa into typeddjango:master May 1, 2024
40 checks passed
@intgr intgr deleted the add-circular-setting-testcase branch May 2, 2024 07:38
@flaeppe
Copy link
Member

flaeppe commented May 6, 2024

It seems that returning a dependency with PRI_HIGH in get_additional_deps for django.conf.settings removes the error emitted in the test case added here.

intgr added a commit that referenced this pull request May 9, 2024
As discussed in #2097 (reply in thread):

* django-stubs 5.0.0 introduces new cases of the "Import cycle from Django settings module prevents type inference" error that did not occur in 4.2.7.
* This was bisected down to the change in commit e517a1f (#2024).
* Updated the priority of internal settings import to mypy's `PRI_HIGH` level.

Related commit a10f8aa (#2098)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants