The "eschew the extraneous else" policy of flake8-eyeo considered too strict

[kvas]

Background

We've implemented a rule proposed by Larry Hastings in ​a lightning talk in our ​Python style checker plugin. Basic idea is that when the code in the body of an if statement would exit the scope, we don't need an else block because the code under will not be reached anyway when if condition is true, so:

def example(number):
    if number > 5:
        return number - 5
    else:
        return number

becomes:

def better_example(number):
    if number > 5:
        return number - 5
    return number

However, not all unnecessary elses are created equal. Consider this code:

    def worse_example(self, value):
        if value == BAD_CASE:
            raise Exception('Bad value')
        elif value == SPECIAL_CASE:
            self.special(value)
        elif value in NORMAL_CASES:
            self.normal(value)
        else:
            raise Exception('Unexpected value')

The first elif here would be considered "extraneous" but the code actually reads better as it is and the logic of 4 mutually exclusive possibilities is clearer this way. However, our style checker plugin will issue a warning for this and demand that the first elif is changed to if.

What to change

Decide what to do with such cases and implement the changes if the policy is to change.

[sebastian]

Based on this constructed example, I'm not convinced if it justifies an exception to the rule. Following alternative looks good to me:

def worse_example(self, value):
    if value == BAD_CASE:
        raise Exception('Bad value')

    if value == SPECIAL_CASE:
        self.special(value)
    elif value in NORMAL_CASES:
        self.normal(value)
    else:
        raise Exception('Unexpected value')

Though, I see your point that it somewhat breaks pattern matching, which however is extremely limited in Python anyway, so that I don't even consider it a supported idiom.

How should a rule, forbidding extraneous else, with an exception for patter matching, look like anyway? Is this even possible, without false positives/negatives, and without too ridicolous logic that cannot only be implemented in a sane way, but also easily be reproduced and remembered by humans?

[kvas]

Just for the record, ​this is the original review where this originated from (see comments on line 83). After looking at it again, I would say that your proposal for the changes would make sense if applied to the real code. Being inside of a fixed string is sort of a special case there and after that we're just doing matching on the tag.

So, this example is not the best but in general I'm not convinced that all "extraneous" elses should be removed -- readability can be tricky and the extraneous else rule is too simple to capture the deep wisdom. On the other hand one can make an argument that in overwhelming majority of the cases the rule improves readability and in the rest of the cases it doesn't make it much worse; having automated style checking is a good thing so on the balance it's worth it. Then of course someone could say that even in majority of cases the readability is not improved. I'd find it rather hard to argue against this since I don't think one is obviously better than the other and readability is pretty subjective (the functional programmer in me definitely wants to keep the elses).

All in all, it seems like I don't feel strong either way so I'm leaning towards doing nothing. Does anyone have further thoughts on this?

[sebastian]

Feel free to provide real world examples, and suggest a concrete rule that addresses them in a way that considers my concerns above. Alternatively, we could discuss, whether the "extraneous else" rule should be removed completely, but the arguments in this regard, seem pretty weak so far.

For reference, we use a ​similar rule when linting JavaScript, as well, now. While it is a different programming languages, with their own idioms, of course, there might still be reasons to do something in a specific way, that applies on a more general level, and therefore if we think it makes sense in one scenario, it probably makes sense in other scenarios too (I'm talking about consistence). Not sure though how much it applies here, since JavaScript (as opposed to Python) has switch/case which you would usually use when doing pattern matching.