-
Notifications
You must be signed in to change notification settings - Fork 710
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Python lint: Use flake8 --extend-ignore instead of --ignore #1498
Conversation
@@ -784,7 +784,7 @@ def GIT(must_succeed=True): | |||
if ret == 0: | |||
cached_git_executable = git | |||
return git | |||
except: | |||
except Exception: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we are trying to catch everything, why not use the shorter form?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this bad?
A bare except
catches BaseException
which includes KeyboardInterrupt
, SystemExit
, Exception
, and others. Catching BaseException
can make it hard to interrupt the program (e.g., with Ctrl-C) and can disguise other problems.
https://peps.python.org/pep-0008/#programming-recommendations
When catching exceptions, mention specific exceptions whenever possible instead of using a bare except: clause:
try:
import platform_specific_module
except ImportError:
platform_specific_module = None
A bare except: clause will catch SystemExit and KeyboardInterrupt exceptions, making it harder to interrupt a program with Control-C, and can disguise other problems. If you want to catch all exceptions that signal program errors, use except Exception: (bare except is equivalent to except BaseException:).
A good rule of thumb is to limit use of bare ‘except’ clauses to two cases:
- If the exception handler will be printing out or logging the traceback; at least the user will be aware that an error has occurred.
- If the code needs to do some cleanup work, but then lets the exception propagate upwards with raise. try...finally can be a better way to handle this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes we don't want to catch KeyboardInterrupt here I guess
W291, W391, W503, W504 | ||
|
||
exclude = | ||
extend-exclude = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This wasn't needed before?
Related to
After re-reading the flake8 configuration docs, I remembered that using
flake8 --extend-ignore
instead of--ignore
greatly reduces the number of active lint rules.