Permit to disable cyclic-import when importing inside a function

[pde]

If two modules import from each other, but one of them only imports the other within functions and other non-global scopes, Python is happy. But pylint still errors on such cyclic imports, and in fact won't accept a module-scoped disable directive, so cyclic import detection has to be disabled for the entire project.

Here's an example from this PR: certbot/certbot#2649 :

git clone https://github.com/letsencrypt/letsencrypt
cd letsencrypt
git checkout split-cli
vim .pylintrc # remove the disable cyclic-import
tools/venv.sh
venv/bin/pylint --rcfile=.pylintrc letsencrypt/cli.py letsencrypt/main.py
************* Module letsencrypt.main
R:  1, 0: Cyclic import (letsencrypt.cli -> letsencrypt.main) (cyclic-import)
...

[The-Compiler]

I think the checker itself is working as intended - cyclic imports can be problematic even when they don't break right away (like when using from-imports instead, or on older Python versions - for example, 3.5 can handle some cyclic imports 3.4 can't).

That being said, I disabled it myself too because it was too noisy. Then again, it only complaining when Python complains anyways would be somewhat pointless.

Maybe as a middle ground, cyclic-import shouldn't get emitted when an import is inside a function, as then clearly the developer is aware they're working around a circular import? Let's see what @PCManticore thinks.

[PCManticore]

I think it makes sense not to emit the error in case one of the imports is inside a function scope, as @The-Compiler already said. Sometimes getting rid of cyclic imports is impossible, requiring redesigning the entire project (as we already have this problem in astroid for example). That being said, I'll probably work on a patch in this weekend, if no one else wants to do it.

[sthenault]

IMHO we should always warn about cyclic import and let the user explicitly
deactivate it rather than adding a specific rule.
Le 18 mars 2016 12:47, "Claudiu Popa" notifications@github.com a écrit :

I think it makes sense not to emit the error in case one of the imports is
inside a function scope, as @The-Compiler
https://github.com/The-Compiler already said. Sometimes getting rid of
cyclic imports is impossible, requiring redesigning the entire project (as
we already have this problem in astroid for example). That being said, I'll
probably work on a patch in this weekend, if no one else wants to do it.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub
#850 (comment)

[PCManticore]

True, but at least they should have some control over disabling the error in a more granular way. My initial thought is to have a flag that can control if the users want to not warn about cyclic-import if one of the imports is protected by a function scope, defaulting to false, so with the same behaviour as now.

[sthenault]

another idea would be that "# disable: cyclic-import" would make the
matched imports unconsidered the cycle computation, breaking cycle if any.

On Fri, Mar 18, 2016 at 7:13 PM, Claudiu Popa notifications@github.com
wrote:

True, but at least they should have some control over disabling the error
in a more granular way. My initial thought is to have a flag that can
control if the users want to not warn about cyclic-import if one of the
imports is protected by a function scope, defaulting to false, so with the
same behaviour as now.

—
You are receiving this because you commented.
Reply to this email directly or view it on GitHub
#850 (comment)

Sylvain

[rogalski]

Discussion looks like a duplicate of #59, with exception of function-scope imports.

[actionless]

mb it could be two different warnings, cyclic-import and cyclic-import-from-function ?

[ParasJain-dev]

@PCManticore Is this issue fixed?
We are still getting this issue in pylint==1.9.*

[Pierre-Sassoulas]

I think the suggestion from sthenault to break cycle on # pylint: disable=cyclic-import is elegant and probably the right way to do this.

[clavedeluna]

I think the suggestion from sthenault to break cycle on # pylint: disable=cyclic-import is elegant and probably the right way to do this.

@Pierre-Sassoulas I think I can try to work on this but I don't think I understand this implementation suggestion. What does it mean to break cycle in this context? Maybe a test case would help.

[Pierre-Sassoulas]

In the algorithm to detect cyclic import we follow the "cycle" of import (https://github.com/PyCQA/pylint/blob/2ae0e98454c28e2aaf1c892655f34eb8d19772d7/pylint/graph.py#L181), if at some point an import is done in a context where cyclic-import is disabled, then we should not take this import into account in the algorithm. We'll need to be able to tell that cyclic import is disabled in a context, which might not be easy (the right place is probably when we generate the graph itself in https://github.com/PyCQA/pylint/blob/2ae0e98454c28e2aaf1c892655f34eb8d19772d7/pylint/checkers/imports.py#L476).

[clavedeluna]

I'm trying to come up with a test case but I think the local disabling is actually working as intended? Try this:

a.py

from b import bye

def hello():
    print("hello")

b.py

def bye():
    print("bye")
    from a import hello

pylint a.py b.py

....
b.py:1:0: R0401: Cyclic import (a -> b) (cyclic-import)

if b.py is updated to

def bye():
    print("bye")
    from a import hello  # pylint: disable=cyclic-import

The cyclic-import warning is gone, which is what we want? Can someone confirm I didn't incorrectly do this? @Pierre-Sassoulas any thoughts?

[jacobtylerwalls]

Yes, getting function level disables to work for cyclic-import was fixed in pylint 1.7 in 2017, see #59.

#6983 (marked as dupe) also discusses whether to enable disabling cyclic-import at the module level, but since that's not discussed on this thread, I think we should close as completed.