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.

Sign up for GitHub

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

Make unused-import check all ancestors for typing guards #5316

Merged
merged 1 commit into from
Nov 25, 2021
Merged

Make unused-import check all ancestors for typing guards #5316

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions pylint/checkers/variables.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
import re
import sys
from functools import lru_cache
from typing import DefaultDict, List, Optional, Set, Tuple
from typing import DefaultDict, List, Optional, Set, Tuple, Union

import astroid
from astroid import nodes
Expand Down Expand Up @@ -358,12 +358,13 @@ def _assigned_locally(name_node):
return any(a.name == name_node.name for a in assign_stmts)


def _is_type_checking_import(node):
parent = node.parent
if not isinstance(parent, nodes.If):
return False
test = parent.test
return test.as_string() in TYPING_TYPE_CHECKS_GUARDS
def _is_type_checking_import(node: Union[nodes.Import, nodes.ImportFrom]) -> bool:
"""Check if an import node is guarded by a TYPE_CHECKS guard"""
for ancestor in node.node_ancestors():
if isinstance(ancestor, nodes.If):
if ancestor.test.as_string() in TYPING_TYPE_CHECKS_GUARDS:
Comment on lines +364 to +365
Copy link
Member

Choose a reason for hiding this comment

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

I'm again late, as I'm going through my notifications.
Can't those two be combined? Technically even a return any(...) would be possible, but I think that might be more difficult to read.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, should have catched that myself. Going to make a PR with this change!

consider-combining-if seems like a handy checker for our review process 😄

Copy link
Member

Choose a reason for hiding this comment

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

consider-combining-if seems like a handy checker for our review process 😄

That is also somewhere on my ideas list 😅

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I found the any to be okay for readability. See #5410

Copy link
Member

Choose a reason for hiding this comment

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

consider-combining-if seems like a handy checker for our review process

Why didn't I think of that sooner, It's like half the content of my reviews 😄 Also it should trigger on thing like any(bool(e == 1) for e in elements if int(e) == e)). Let me open an issue.

Copy link
Member

Choose a reason for hiding this comment

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

#5414

return True
return False
Pierre-Sassoulas marked this conversation as resolved.
Show resolved Hide resolved


def _has_locals_call_after_node(stmt, scope):
Expand Down
4 changes: 4 additions & 0 deletions tests/functional/u/unused/unused_import.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,7 @@ def dummy(self, truc):
def blop(self):
"""yo"""
print(self, 'blip')

if TYPE_CHECKING:
if sys.version_info >= (3, 6, 2):
from typing import NoReturn