[Adjust Rule] SIM106: Error cases first only if it's not "NotImplementedError"

[MartinThoma]

Desired change

• Rule(s): SIM106
• Adjustment: Make an exception for the NotImplementedError

Explanation

Throwing an exception for potentially forgotten implementation is better than potentially returning None or making the last one catch all.

Example

    if merge_method in ["take_right_shallow", "take_right_deep"]:
        ...
    elif merge_method == "take_left_shallow":
        ...
    elif merge_method == "take_left_deep":
        ...
    elif merge_method == "sum":
        ...
    else:
        raise NotImplementedError

[ROpdebee]

My take on this is that it should be allowed to raise an exception in an else block when there's a chain of ifs, regardless of the type of exception raised. Take for instance:

if path.is_dir():
   ...
elif path.is_file():
   ...
else:
   raise ValueError('File or directory expected')

Fail-fast in this case would be to do

if not (path.is_file() or path.is_dir()):
   raise ValueError('File or directory expected')

if path.is_file():
   ...
elif path.is_dir():
   ...

which makes it more complicated, IMO.

In case there's no elifs, e.g.

if path.is_file():
   ...
else:
   raise NotImplementedError

IMHO it's clearer to just do

if not path.is_file():
   raise NotImplementedError

# We're sure it's a file
...

although the former would be allowed by the proposed adjustment.

A final note (perhaps a bit outside the scope of this issue) is the following:

if path.is_file():
    ...
    return something

raise NotImplementedError

That's also not detected at the moment, but detecting such things might be a bit more complicated.

[MartinThoma]

Hey, good points! Thank you for taking the time and sharing this.

At the moment, I'm too busy with other stuff. I don't think I can work in this in the next 2 weeks. But I will do it in November :-)

[MartinThoma]

The final note is actually also fine, I think. I personally can read the "else" version better, but I can understand if people want the raise to be at the very end like this.

I've just had a look at this issue. I've created a test, but it's not easy to directly solve it. Essentially, the Visitor would need to remember something about the parent nodes (or at least be able to look up again). I don't know if that is possible with the current architecture at all.

[MartinThoma]

This issue is blocked by #21

[MinmoTech]

Since the blocking issue is resolved, is there any progress here? :)