-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Fix return type checkers calls on ellipses functions #5107
Conversation
Pull Request Test Coverage Report for Build 1312511792
💛 - Coveralls |
pylint/checkers/classes.py
Outdated
@staticmethod | ||
def _function_body_is_ellipsis(node: nodes.FunctionDef) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about moving this method to checkers/utils.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be in astroid's FunctionDef instead ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be in astroid's FunctionDef instead ?
Though about this some more. I'm not sure we should move these kinds of helper functions to astroid
. astroid
only really deals with the ast
and inference, everything else should be done by pylint
.
By the same logic I believe it might have been wrong to move is_sys_guard
and is_typing_guard
to astroid
in the first place. They don't really belong there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't it be argued that these methods are part of the inference? In that they give additional information about the specific type of FunctionDef
node?
Anyway, I'll leave this up to you and Pierre. I'm fine with either.
If wanted I can also make a PR to re-move is_sys_guard
and is_typing_guard
to pylint
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't it be argued that these methods are part of the inference? In that they give additional information about the specific type of
FunctionDef
node?
True. Personally I would consider this a case of meta inference. We check multiple conditions to decide wether or not to emit a warning. That's that sort of thing pylint
does all the time.
One thing to consider with that, it's much easier to iterate (read: fix) such a check in pylint
alone than to modify something in astroid
+ pylint
.
What I do consider useful in astroid
are helpers like this one pylint-dev/astroid#1169 which helps dealing with the NodeNG
tree itself.
Anyway, I'll leave this up to you and Pierre. I'm fine with either.
If wanted I can also make a PR to re-moveis_sys_guard
andis_typing_guard
topylint
.
I would be in favor of it, but let's see what @Pierre-Sassoulas thinks first. Unfortunately, we can't outright remove them now, so we would need to deprecate them first (and remove with astroid 3.0).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's wait for this to release astroid 2.8.1 then. My local clone is broken and I need to fix it anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mean first release 2.8.1
and afterwards deprecate, or the other way around?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, let's do the deprecation now. I.e. wait with releasing astroid 2.8.1
until it's done.
pylint-dev/astroid#1199
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened pylint-dev/astroid#1202
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #5122 to add the guard helper functions back to pylint.
Co-authored-by: Marc Mueller <[email protected]>
The "invalid-*-returned" messages shouldn't be emitted for stub functions | ||
Original issue: https://github.com/PyCQA/pylint/issues/4736""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move these just before the class definition? Someone might add additional test cases later on. That way at least the general docstring will stay up to date.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR could be merged after the last comment is addressed and the merge conflict is resolved.
Should be good to go now! |
doc/whatsnew/<current release.rst>
.Type of Changes
Description
Closes #4736