pylint: Else clause on a loop without break statement
Hello, often I write code like this: def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): return False if expression.isMutable(): return False else: return True That is to search in an iterable, and return based on finding something, or returning in the alternative, I specifically prefer the "else:" branch over merely putting it after the "for" loop. This triggers the above message, which I consider flawed, because as soon as there is a "raise", or "return", that should be good enough as well. Basically any aborting statement, not only break. I wanted to hear your opinion on this, pylint bug, or only in my mind. Yours, Kay
I don't see any mesage from pylint in your email, could you post it again? Regardless, I think what you're disagreeing with is the language specification. The documentation specifies that the `else` is only triggered on breaks: http://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statem... On Sun, Jan 12, 2014 at 8:57 AM, Kay Hayen <kay.hayen@gmail.com> wrote:
Hello,
often I write code like this:
def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): return False
if expression.isMutable(): return False else: return True
That is to search in an iterable, and return based on finding something, or returning in the alternative, I specifically prefer the "else:" branch over merely putting it after the "for" loop.
This triggers the above message, which I consider flawed, because as soon as there is a "raise", or "return", that should be good enough as well. Basically any aborting statement, not only break.
I wanted to hear your opinion on this, pylint bug, or only in my mind.
Yours, Kay _______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
Ian, it's the opposite - 'else' is only triggered if *no* break statement is encountered. On 12 January 2014 16:12, Ian Cordasco <graffatcolmingov@gmail.com> wrote:
I don't see any mesage from pylint in your email, could you post it again?
Regardless, I think what you're disagreeing with is the language specification. The documentation specifies that the `else` is only triggered on breaks:
http://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statem...
On Sun, Jan 12, 2014 at 8:57 AM, Kay Hayen <kay.hayen@gmail.com> wrote:
Hello,
often I write code like this:
def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): return False
if expression.isMutable(): return False else: return True
That is to search in an iterable, and return based on finding something,
or
returning in the alternative, I specifically prefer the "else:" branch over merely putting it after the "for" loop.
This triggers the above message, which I consider flawed, because as soon as there is a "raise", or "return", that should be good enough as well. Basically any aborting statement, not only break.
I wanted to hear your opinion on this, pylint bug, or only in my mind.
Yours, Kay _______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
Woops. Sorry for the confusion. I never end up using that particular construct so I never get it right. On Sun, Jan 12, 2014 at 9:18 AM, Carl Crowder <carl.crowder@gmail.com> wrote:
Ian, it's the opposite - 'else' is only triggered if *no* break statement is encountered.
On 12 January 2014 16:12, Ian Cordasco <graffatcolmingov@gmail.com> wrote:
I don't see any mesage from pylint in your email, could you post it again?
Regardless, I think what you're disagreeing with is the language specification. The documentation specifies that the `else` is only triggered on breaks:
http://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statem...
On Sun, Jan 12, 2014 at 8:57 AM, Kay Hayen <kay.hayen@gmail.com> wrote:
Hello,
often I write code like this:
def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): return False
if expression.isMutable(): return False else: return True
That is to search in an iterable, and return based on finding something, or returning in the alternative, I specifically prefer the "else:" branch over merely putting it after the "for" loop.
This triggers the above message, which I consider flawed, because as soon as there is a "raise", or "return", that should be good enough as well. Basically any aborting statement, not only break.
I wanted to hear your opinion on this, pylint bug, or only in my mind.
Yours, Kay _______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
_______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
def _areConstants(expressions): return all(_isConstant(expression) for expression in expressions) def _isConstant(expression): return expression.isExpressionConstantRef() and not expression.isMutable() ? When I tried to understand the OP's code, I had to mentally step through the code and assemble the meaning, whereas the above code requires no such work (it probably takes a bit longer per-line, but less time over-all). If you're asking about whether pylint should complain about the OP's code, I think that it shouldn't -- in the semantics of for/else, return and break are similar and rewriting OP's code to suppress the warnings is not an improvement: def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): break if expression.isMutable(): break else: return True return False (Caveat: none of this code has been tested) On 12 January 2014 07:21, Ian Cordasco <graffatcolmingov@gmail.com> wrote:
Woops. Sorry for the confusion. I never end up using that particular construct so I never get it right.
On Sun, Jan 12, 2014 at 9:18 AM, Carl Crowder <carl.crowder@gmail.com> wrote:
Ian, it's the opposite - 'else' is only triggered if *no* break statement is encountered.
On 12 January 2014 16:12, Ian Cordasco <graffatcolmingov@gmail.com> wrote:
I don't see any mesage from pylint in your email, could you post it
again?
Regardless, I think what you're disagreeing with is the language specification. The documentation specifies that the `else` is only triggered on breaks:
http://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statem...
On Sun, Jan 12, 2014 at 8:57 AM, Kay Hayen <kay.hayen@gmail.com> wrote:
Hello,
often I write code like this:
def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): return False
if expression.isMutable(): return False else: return True
That is to search in an iterable, and return based on finding
something,
or returning in the alternative, I specifically prefer the "else:" branch over merely putting it after the "for" loop.
This triggers the above message, which I consider flawed, because as soon as there is a "raise", or "return", that should be good enough as well. Basically any aborting statement, not only break.
I wanted to hear your opinion on this, pylint bug, or only in my mind.
Yours, Kay _______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
_______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
I think the point is that Pylint does not only say "this is wrong", but also says "are you sure this is right?". These things are usually warnings but perhaps 'code smell' is a better name. Because, in this case, the 'else' isn't strictly necessary, Pylint (correctly, in my opinion) raises a warning which effectively says "This 'else' clause does not actually need to be there - did you do it on purpose, or have some break statements been refactored away or something?". I consider it more like a code review, in which the reviewer tentatively asks "this looks odd - is it deliberate?" simply to verify in case it was not. It's pretty easy to suppress the warning either on this line alone or on the entire project if this is your code style, so I prefer the case where Pylint catches a real error but may be to hasty for some users. I think Pylint should be regarded as producing both actual errors as output but also advisories and questions. On 12.01.2014, at 18:45, Peter Ludemann <pludemann@google.com> wrote:
def _areConstants(expressions): return all(_isConstant(expression) for expression in expressions)
def _isConstant(expression): return expression.isExpressionConstantRef() and not expression.isMutable()
?
When I tried to understand the OP's code, I had to mentally step through the code and assemble the meaning, whereas the above code requires no such work (it probably takes a bit longer per-line, but less time over-all).
If you're asking about whether pylint should complain about the OP's code, I think that it shouldn't -- in the semantics of for/else, return and break are similar and rewriting OP's code to suppress the warnings is not an improvement:
def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): break if expression.isMutable(): break else: return True return False
(Caveat: none of this code has been tested)
On 12 January 2014 07:21, Ian Cordasco <graffatcolmingov@gmail.com> wrote: Woops. Sorry for the confusion. I never end up using that particular construct so I never get it right.
On Sun, Jan 12, 2014 at 9:18 AM, Carl Crowder <carl.crowder@gmail.com> wrote:
Ian, it's the opposite - 'else' is only triggered if *no* break statement is encountered.
On 12 January 2014 16:12, Ian Cordasco <graffatcolmingov@gmail.com> wrote:
I don't see any mesage from pylint in your email, could you post it again?
Regardless, I think what you're disagreeing with is the language specification. The documentation specifies that the `else` is only triggered on breaks:
http://docs.python.org/2/tutorial/controlflow.html#break-and-continue-statem...
On Sun, Jan 12, 2014 at 8:57 AM, Kay Hayen <kay.hayen@gmail.com> wrote:
Hello,
often I write code like this:
def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): return False
if expression.isMutable(): return False else: return True
That is to search in an iterable, and return based on finding something, or returning in the alternative, I specifically prefer the "else:" branch over merely putting it after the "for" loop.
This triggers the above message, which I consider flawed, because as soon as there is a "raise", or "return", that should be good enough as well. Basically any aborting statement, not only break.
I wanted to hear your opinion on this, pylint bug, or only in my mind.
Yours, Kay _______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
_______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
_______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
On 12 janvier 19:04, Carl Crowder wrote:
I think the point is that Pylint does not only say "this is wrong", but also says "are you sure this is right?". These things are usually warnings but perhaps 'code smell' is a better name. Because, in this case, the 'else' isn't strictly necessary, Pylint (correctly, in my opinion) raises a warning which effectively says "This 'else' clause does not actually need to be there - did you do it on purpose, or have some break statements been refactored away or something?". I consider it more like a code review, in which the reviewer tentatively asks "this looks odd - is it deliberate?" simply to verify in case it was not.
It's pretty easy to suppress the warning either on this line alone or on the entire project if this is your code style, so I prefer the case where Pylint catches a real error but may be to hasty for some users. I think Pylint should be regarded as producing both actual errors as output but also advisories and questions.
That's exactly the point: pylint does only tell you "buddy, there may be here something weird or/and that could be written differently", and that's perfectly fine to shut him down if you feel it's ok. -- Sylvain Thénault, LOGILAB, Paris (01.45.32.03.12) - Toulouse (05.62.17.16.42) Formations Python, Debian, Méth. Agiles: http://www.logilab.fr/formations Développement logiciel sur mesure: http://www.logilab.fr/services CubicWeb, the semantic web framework: http://www.cubicweb.org
On 12/01/14 18:45, Peter Ludemann wrote:
If you're asking about whether pylint should complain about the OP's code, I think that it shouldn't -- in the semantics of for/else, return and break are similar and rewriting OP's code to suppress the warnings is not an improvement:
def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): break if expression.isMutable(): break else: return True return False
That's not an improvement but also not the obvious way to rewrite the code to suppress the, IMHO legitimate, warning. Instead of introducing ``break``\s for an unnecessary ``else`` clause one could also just remove that unnecessary ``else``:: def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): return False if expression.isMutable(): return False return True Which improves the situation in a way, because now the fellow Python coder doesn't wonder where the ``break`` should be or if the author understood the semantics of ``else`` on loop constructs. I would also avoid this question by using `all()` here. :-) Ciao, Marc 'BlackJack' Rintsch -- “It was not a good idea to address any prayers to a Supreme Being. It would only attract his attention and might cause trouble.” -- Terry Pratchett, Small Gods
Hello, thanks for all the replies. Of course I am aware that my use of the "else:" is different from the "break" case when it comes to "return". For return, the "else:" is not needed, as it won't continue the execution.
def _areConstants(expressions):
for expression in expressions: if not expression.isExpressionConstantRef(): break if expression.isMutable(): break else: return True return False
That's not an improvement but also not the obvious way to rewrite the code to suppress the, IMHO legitimate, warning. Instead of introducing ``break``\s for an unnecessary ``else`` clause one could also just remove that unnecessary ``else``::
Mind you, I am using the "else:" specifically to express, that I expect the loop to return based on one element. I agree with you that the suggested code is making that hard to discern and that removing the "else" clause is an option. The thing is, I developed a style, where a return in the loop always leads to a return in a else. It's the pick and choose method. So any time, I make decisions based on an iterable, I do it like that. def _areConstants(expressions):
for expression in expressions: if not expression.isExpressionConstantRef(): return False if expression.isMutable(): return False return True
Which improves the situation in a way, because now the fellow Python coder doesn't wonder where the ``break`` should be or if the author understood the semantics of ``else`` on loop constructs.
That precisely is the question. Is the "else" an emphasis, or is it an error indicator. I can assure you that I did it on purpose. But if nobody gets that, it kinds of misses the point. I take the general feedback to say "yes, using else: without need is a style problem". So I will try and give it up. :-)
I would also avoid this question by using `all()` here. :-)
I learned of "any" and "all" relatively late. I agree for booleans it's the better choice, but it's another subject. Many times, it's not a boolean return value. Yours, Kay
On 12 January 2014 16:17, Kay Hayen <kay.hayen@gmail.com> wrote:
Hello,
thanks for all the replies.
Of course I am aware that my use of the "else:" is different from the "break" case when it comes to "return". For return, the "else:" is not needed, as it won't continue the execution.
def _areConstants(expressions):
for expression in expressions: if not expression.isExpressionConstantRef(): break if expression.isMutable(): break else: return True return False
That's not an improvement but also not the obvious way to rewrite the code to suppress the, IMHO legitimate, warning. Instead of introducing ``break``\s for an unnecessary ``else`` clause one could also just remove that unnecessary ``else``::
Mind you, I am using the "else:" specifically to express, that I expect the loop to return based on one element. I agree with you that the suggested code is making that hard to discern and that removing the "else" clause is an option.
I'm reversing my earlier opinion ... if all the ways of breaking out of the loop are "return", the "else" is not needed and Pylint should point this out.
The thing is, I developed a style, where a return in the loop always leads to a return in a else. It's the pick and choose method. So any time, I make decisions based on an iterable, I do it like that.
def _areConstants(expressions):
for expression in expressions: if not expression.isExpressionConstantRef(): return False if expression.isMutable(): return False return True
Which improves the situation in a way, because now the fellow Python coder doesn't wonder where the ``break`` should be or if the author understood the semantics of ``else`` on loop constructs.
That precisely is the question. Is the "else" an emphasis, or is it an error indicator. I can assure you that I did it on purpose. But if nobody gets that, it kinds of misses the point.
I take the general feedback to say "yes, using else: without need is a style problem". So I will try and give it up. :-)
I would also avoid this question by using `all()` here. :-)
I learned of "any" and "all" relatively late. I agree for booleans it's the better choice, but it's another subject. Many times, it's not a boolean return value.
"Any" and "all" are just special cases of "max" and "min". ;) And there are many other tools for processing lists without for-loops, e.g.: http://docs.python.org/3/library/itertools.html#itertools-recipes - peter
Yours, Kay
_______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
This came up once before, and I think the reasoning is that a for loop without a break statement means the 'else' is redundant. In your example, you can remove the 'else' and it would be functionally the same. On 12 January 2014 15:57, Kay Hayen <kay.hayen@gmail.com> wrote:
Hello,
often I write code like this:
def _areConstants(expressions): for expression in expressions: if not expression.isExpressionConstantRef(): return False
if expression.isMutable(): return False else: return True
That is to search in an iterable, and return based on finding something, or returning in the alternative, I specifically prefer the "else:" branch over merely putting it after the "for" loop.
This triggers the above message, which I consider flawed, because as soon as there is a "raise", or "return", that should be good enough as well. Basically any aborting statement, not only break.
I wanted to hear your opinion on this, pylint bug, or only in my mind.
Yours, Kay _______________________________________________ code-quality mailing list code-quality@python.org https://mail.python.org/mailman/listinfo/code-quality
participants (6)
-
Carl Crowder -
Ian Cordasco -
Kay Hayen -
Marc 'BlackJack' Rintsch -
Peter Ludemann -
Sylvain Thénault