-
Notifications
You must be signed in to change notification settings - Fork 107
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
Implement B037 check for yielding or returning values in __init__() #442
Conversation
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.
LGTM - But I think I would like if we allowed return None
unless it's difficult that I'm not seeing ...
I'm a big explicit is better than implicit person ...
class D(C): | ||
def __init__(self, k="") -> None: | ||
super().__init__(k) | ||
return None # bad |
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 can see people disliking this due to
Explicit is better than implicit
In the Zen of Python.
Can we just check if a None
is being explicitly returned and not error in that case since we allow return
...
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.
Yeah it's easy enough to do if you prefer, but my argument against allowing return None
is that, while return
is functionally equivalent to return None
in python, the latter should only be used in cases where you might return something (like a C function that returns either a valid pointer or NULL
). Whereas just bare return
is ok, when used as a early exit point from a function (like a C void function).
some quick examples:
def get_thing() -> Thing | None:
"""return a thing if possible"""
for thing in things:
if condition:
return thing
return None # this line could be omitted, but functions that have a non-empty return
# in some case should have non-empty return in all cases (pylint would
# complain about this for example)
vs
class Example:
def __init__(self, size, do_extra_stuff = False):
self.x = ...
if not do_extra_stuff:
return # early exit. Could do `return None` but that implies
# that we might return something somewhere else?
# do some more stuff
let me know what you think, thanks!
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 agree that it's better to lint against return None
.
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.
Since this is __init__
only, I can live with it. I'll have code to 'fix' cause of it.
bugbear.py
Outdated
@@ -349,6 +351,35 @@ def visit_ExceptHandler(self, node: ast.ExceptHandler): | |||
return super().generic_visit(node) | |||
|
|||
|
|||
class ClassInitVisitor(ast.NodeVisitor): |
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.
Might be cleaner to put this inside the BugBearVisitor
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.
Done, LMK if that looks better
Waiting on anything else here? |
class D(C): | ||
def __init__(self, k="") -> None: | ||
super().__init__(k) | ||
return None # bad |
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.
Since this is __init__
only, I can live with it. I'll have code to 'fix' cause of it.
Implement check for
return <value>
,yield
,yield <value>
, oryield from <value>
in class__init__()
method. No values should be returned or yielded, only barereturn
s are ok.These would be runtime errors, if and when these statements are reached.
This does trigger on
return None
which is equivalent toreturn
, but kind of confusing and pointless to write in a function that shouldn't return anything.