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

Remove most typing.cast() calls #4995

Merged
merged 3 commits into from Sep 15, 2021
Merged

Conversation

DanielNoord
Copy link
Member

@DanielNoord DanielNoord commented Sep 13, 2021

  • Add yourself to CONTRIBUTORS if you are a new contributor.
  • Write a good description on what the PR does.

Type of Changes

Type
βœ“ πŸ”¨ Refactoring

Description

After merge of PyCQA/astroid#1168 I was asked to work on this.
I left in three two calls.

One and two (because mypy/IDE fails to see L120-122):

if len(node.items) > 1 and all(
isinstance(dict_value, nodes.Dict) for _, dict_value in node.items
):
KeyTupleT = Tuple[Type[nodes.NodeNG], str]
# Makes sure all keys are 'Const' string nodes
keys_checked: Set[KeyTupleT] = set()
for _, dict_value in node.items:
dict_value = cast(nodes.Dict, dict_value)
for key, _ in dict_value.items:
key_tuple = (type(key), key.as_string())
if key_tuple in keys_checked:
continue
inferred = safe_infer(key)
if not (
isinstance(inferred, nodes.Const)
and inferred.pytype() == "builtins.str"
):
return
keys_checked.add(key_tuple)
# Makes sure all subdicts have at least 1 common key
key_tuples: List[Tuple[KeyTupleT, ...]] = []
for _, dict_value in node.items:
dict_value = cast(nodes.Dict, dict_value)
key_tuples.append(
tuple((type(key), key.as_string()) for key, _ in dict_value.items)
)

Three (because NodeNG.scope() is not typed and we therefore lose typing on scope):

scope = node.scope()
for loop_node in scope.nodes_of_class((nodes.For, nodes.While)):
loop_node = cast(nodes.NodeNG, loop_node)

@coveralls
Copy link

coveralls commented Sep 13, 2021

Pull Request Test Coverage Report for Build 1237960833

  • 3 of 3 (100.0%) changed or added relevant lines in 3 files are covered.
  • 3 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+0.002%) to 93.077%

Files with Coverage Reduction New Missed Lines %
pylint/checkers/refactoring/refactoring_checker.py 1 97.93%
pylint/checkers/refactoring/recommendation_checker.py 2 96.09%
Totals Coverage Status
Change from base Build 1236811898: 0.002%
Covered Lines: 13243
Relevant Lines: 14228

πŸ’› - Coveralls

@cdce8p
Copy link
Member

cdce8p commented Sep 13, 2021

The cast in code_checker.py are necessary and thus cannot be removed unfortunately.

--
Regarding the recommendation_checker.py and scope:
I would recommend to move the cast call in front of the for loop. After that, loop_node should be detected correctly. I.e.

scope = cast(nodes.NodeNG, node.scope())

@Pierre-Sassoulas Pierre-Sassoulas added the Maintenance Discussion or action around maintaining pylint or the dev workflow label Sep 13, 2021
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.11.0 milestone Sep 13, 2021
@DanielNoord
Copy link
Member Author

The cast in code_checker.py are necessary and thus cannot be removed unfortunately.

I think this is a typo. I didn't change code_checker.py, which file are you referring to?

Regarding the recommendation_checker.py and scope:

Can we use cast to type scope to Union[Module, FunctionDef, ClassDef, Lambda, GeneratorExpr]? If we are changing things we might as well be as specific as possible.
I took a quick look at typing NodeNG.scope() in astroid but this isn't a 2-minute fix because node.parent is Optional[NodeNG], thus causing mypy issue.

@Pierre-Sassoulas Pierre-Sassoulas added the Needs astroid update Needs an astroid update (probably a release too) before being mergable label Sep 13, 2021
@cdce8p
Copy link
Member

cdce8p commented Sep 13, 2021

I think this is a typo. I didn't change code_checker.py, which file are you referring to?

You mentioned it in the PR description. Only wanted to comment on that.

Can we use cast to type scope to Union[Module, FunctionDef, ClassDef, Lambda, GeneratorExpr]? If we are changing things we might as well be as specific as possible.

I wouldn't recommend that. My rule for cast is usually to choose the most basic type that is still enough. If the implementation changes (it likely won't in this case), it's more likely that the cast will still be accurate.

I took a quick look at typing NodeNG.scope() in astroid but this isn't a 2-minute fix because node.parent is Optional[NodeNG], thus causing mypy issue.

Left some comments on PyCQA/astroid#1170.

@DanielNoord
Copy link
Member Author

You mentioned it in the PR description. Only wanted to comment on that.

Ah, that was code_style.py. But yeah, can't do anything about it.

I wouldn't recommend that. My rule for cast is usually to choose the most basic type that is still enough. If the implementation changes (it likely won't in this case), it's more likely that the cast will still be accurate.

πŸ‘πŸ»

@DanielNoord
Copy link
Member Author

This is (potentially) blocked by PyCQA/astroid#1170 as it would allow removing one additional cast() call.

@Pierre-Sassoulas
Copy link
Member

This is (potentially) blocked by PyCQA/astroid#1170 as it would allow removing one additional cast() call.

We can wait for astroid 2.8, then, there's only two merge request to handle now : https://github.com/PyCQA/astroid/milestone/16

@cdce8p cdce8p marked this pull request as draft September 13, 2021 22:44
@cdce8p
Copy link
Member

cdce8p commented Sep 13, 2021

Temporarily marked it as draft. Just until the PR is merged

@Pierre-Sassoulas Pierre-Sassoulas marked this pull request as ready for review September 14, 2021 17:27
Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

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

LGTM !

@DanielNoord
Copy link
Member Author

Still blocked by PyCQA/astroid#1170

@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.11.0 milestone Sep 14, 2021
@cdce8p cdce8p marked this pull request as draft September 14, 2021 18:29
@cdce8p cdce8p added the Blocked 🚧 Blocked by a particular issue label Sep 15, 2021
@DanielNoord DanielNoord marked this pull request as ready for review September 15, 2021 14:12
@cdce8p cdce8p removed the Blocked 🚧 Blocked by a particular issue label Sep 15, 2021
cdce8p
cdce8p approved these changes Sep 15, 2021
@Pierre-Sassoulas Pierre-Sassoulas merged commit cb89612 into PyCQA:main Sep 15, 2021
32 checks passed
@DanielNoord DanielNoord deleted the typing-cast branch September 15, 2021 17:55
@DanielNoord
Copy link
Member Author

DanielNoord commented Sep 15, 2021

This does need astroid 2.8.1, but that shouldn't be a problem for now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Maintenance Discussion or action around maintaining pylint or the dev workflow Needs astroid update Needs an astroid update (probably a release too) before being mergable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants