Closed MartinThoma closed 2 years ago
My take on this is that it should be allowed to raise an exception in an else
block when there's a chain of if
s, 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 elif
s, 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.
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 :-)
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.
This issue is blocked by https://github.com/MartinThoma/flake8-simplify/issues/21
Since the blocking issue is resolved, is there any progress here? :)
Desired change
Explanation
Throwing an exception for potentially forgotten implementation is better than potentially returning None or making the last one catch all.
Example