Adding a check for inconsistent-return-statements inside function or methods.
#1641
Conversation
…eletion of _has_function_explicit_return_statement static method
…stent return statements
… they are legitimate
There was a problem hiding this comment.
This is looking pretty good! Thanks for the change.
One final comment, please could you add complete docstrings where appropriate. So document the arguments, their types, and and returns as well. We have a mix of docstring styles. I'd prefer Google style but if you're more comfortable with sphinx style then feel free to use that.
pylint/checkers/refactoring.py
Outdated
| @@ -27,6 +27,52 @@ def _all_elements_are_true(gen): | |||
| return values and all(values) | |||
|
|
|||
|
|
|||
| def _get_exception_handlers(node, exception): | |||
There was a problem hiding this comment.
pylint.checkers.utils.node_ignores_exception is doing something every similar to this, except that it's returning a boolean instead of the list of handlers. So that we're doing handler detection consistently, please could you split utils.node_ignores_exception into a utils.get_exception_handlers function, and then utils.node_ignores_exception can call this and return True if there are any exception handlers.
There was a problem hiding this comment.
Thanks @AWhetter for this review. In fact i get inspired from pylint.checkers.utils.node_ignores_exception. I will do what you are asking asap.
pylint/checkers/refactoring.py
Outdated
| # if statement is returning if there are exactly two return statements in its | ||
| # children : one for the body part, the other for the orelse part | ||
| return_stmts = [_is_node_return_ended(_child) for _child in node.get_children()] | ||
| return_stmts = [_rs for _rs in return_stmts if _rs] |
There was a problem hiding this comment.
Trivial: You could simplify this slightly by removing this line and using return sum(return_stmts) == 2. Or instead keep this line and make the line below return len(return_stmts) == 2.
…handlers. Conditionnal around return_stmts has been simplified
…nores_exception method by using the previous one
pylint/checkers/refactoring.py
Outdated
| self._return_nodes[node.name] = [_rnode for _rnode in return_nodes | ||
| if _rnode.frame() == node.frame()] | ||
|
|
||
| def _check_consistent_returns(self, node): # pylint: disable=redundant-returns-doc |
There was a problem hiding this comment.
This is raising a redundant-returns-doc because you have a returns section in a function that doesn't return anything. Remove the returns section and then you will be able to remove this disable.
AWhetter
added
the
Needs review 🔍
pylint/checkers/classes.py
Outdated
| @@ -287,7 +287,7 @@ def _has_bare_super_call(fundef_node): | |||
| return False | |||
|
|
|||
|
|
|||
| def _safe_infer_call_result(node, caller, context=None): | |||
| def _safe_infer_call_result(node, caller, context=None): # pylint: disable=inconsistent-return-statements | |||
There was a problem hiding this comment.
I would prefer to just fix the occurences of this new check rather than disable them
pylint/checkers/refactoring.py
Outdated
| # but if the exception raised is handled, then the handler has to | ||
| # ends with a return statement | ||
| exc = utils.safe_infer(node.exc) | ||
| exc_name = exc.pytype().split('.')[-1] |
There was a problem hiding this comment.
exc can be None or Uninferable, please check against that.
PCManticore
added
reviewed-waiting-updates
and removed
Needs review 🔍
…(Except for where it really doesn't make sense.
…(Except for where it really doesn't make sense.
|
Thank you @hippo91 ! One thing I forgot to ask you was to add yourself to Contributors file. Feel free to send a PR for that when you get a chance. Thanks a lot for your contributions! |
|
No pb. In fact you asked me to do it in PR #1618. I'm proud to help a
little bit such a great soft! Thanks to you and others (@rogalski,
@AWhetter...) for your time and investment!
Le 26 sept. 2017 5:04 PM, "Claudiu Popa" <[email protected]> a
écrit :
… Thank you @hippo91 <https://github.com/hippo91> ! One thing I forgot to
ask you was to add yourself to Contributors file. Feel free to send a PR
for that when you get a chance. Thanks a lot for your contributions!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1641 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ALJzX-ujZodYiXeOhJGcU1vzQc6VCAClks5smRKGgaJpZM4PCjhw>
.
|
The code is quite simple. I'followed the recommendations of @AWhetter. However the most difficult part
was to determine if a function has an implicit return statement. This is done inside the recursive
_is_node_return_endedfunction. I'm not sure it can handle all encountered cases but the associated unit test is quite extensive...Close #1267