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

Support selectively disabling reportImportCycle #3489

Closed
brandon-leapyear opened this issue May 20, 2022 · 5 comments
Closed

Support selectively disabling reportImportCycle #3489

brandon-leapyear opened this issue May 20, 2022 · 5 comments
Labels
as designed Not a bug, working as intended enhancement request New feature or request

Comments

@brandon-leapyear
Copy link

brandon-leapyear commented May 20, 2022

Is your feature request related to a problem? Please describe.
There have been multiple issues opened around this topic: #746, #1825, #3278. And FWIW, I agree with most of the comments, except for:

The solution I use in this case is to place model classes in the same file if they have mutual recursive dependencies.

In our codebase, where we have ~5 models that are ~500 lines each, it'd really be a pain to maintain them all in a single file. Currently, the following code does work:

# mymodule/foo.py

class Foo:
    def bars(self):
        from mymodule.bar import Bar
        return [Bar(), Bar()]
# mymodule/bar.py

class Bar:
    def foos(self):
        from mymodule.foo import Foo
        return [Foo(), Foo()]

The problem comes when typing the return types. Currently, the best solution I can think of is

# mymodule/foo.py
import mymodule

class Foo:
    def bars(self) -> list["mymodule.bar.Bar"]:
        from mymodule.bar import Bar
        return [Bar(), Bar()]

But this fails with "Cycle detected in import chain", with no way to say "This import mymodule line creates the cycle, but I want to explicitly allow this".

Aside from putting everything into one module, it seems like the only other alternative today is to disable this diagnostic check entirely (or exclude mymodule/__init__.py from being checked by pyright). But in general, we do like this check, just want to disable it in select locations where we know it to be safe.

Describe the solution you'd like
It would be great for

import mymodule  # pyright: ignore[reportImportCycle]

to Just Work.

Additional context

@brandon-leapyear brandon-leapyear added the enhancement request New feature or request label May 20, 2022
@hmc-cs-mdrissi
Copy link

hmc-cs-mdrissi commented May 21, 2022

# pyright: reportImportCycles=false

works.

pyright ignore/type ignore target a specific line and this error does not seem to be attached to any line number.

@brandonchinn178
Copy link

Thanks! I'll try it out. But it would also be nice to be able to specify a specific line. I would imagine the import cycle detector parses and follows import statements, so it's theoretically possible to tag a line to not follow the import

@erictraut
Copy link
Collaborator

Most of the diagnostic rules in pyright operate at the source file level. The reportImportCycles is the exception, as it operates at the "program" level. The program builds an import dependency graph of source files, and the reportImportCycles logic operates on this graph. By the time this logic runs, information about individual import statements within each file are no longer available.

As @hmc-cs-mdrissi mentioned, you can suppress the reportImportCycles output for a specific file using a # pyright: reportImportCycles=false comment within that file, but note that changes to your source could cause the error to move to another file within the cycle. When an import cycle is detected, it is reported in one of the files within that cycle, somewhat arbitrarily.

We don't have plans to change the way the import cycle detection works, so if you intend to leave some cycles in your program but still want to leave the reportImportCycles rule enabled, then the file-level suppression comment is the best solution I can offer you. I would, however, encourage you to get creative and consider ways to eliminate those cycles entirely within your code base.

@erictraut erictraut added the as designed Not a bug, working as intended label May 21, 2022
@brandon-leapyear
Copy link
Author

brandon-leapyear commented May 21, 2022 via email

@erictraut
Copy link
Collaborator

That would involve a pretty major change to the design. As I said, we don't have plans to change how this works at this time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
as designed Not a bug, working as intended enhancement request New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants