-
Notifications
You must be signed in to change notification settings - Fork 38.7k
lint: enable E722 do not use bare except #25867
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
Conversation
luke-jr
left a comment
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.
Concept ACK, but bare excepts should be replaced with specific exceptions, not except Exception.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
9b398c0 to
0b4f624
Compare
|
@luke-jr @aureleoules test/functional/test_framework/p2p.py +383 code is very dynamic. Thanks in advance! |
aureleoules
left a comment
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.
ACK 0b4f62415a4d141670de119d479fdebbd16a054f - LGTM
|
@MarcoFalke merge or close? |
I wonder if this should be done. At least it is unclear to me how to review this easily. And if it is hard to review and maintain, I wonder if it is worth it. |
|
Thanks for reviewing my code. I've identified a bare except statement that could catch exceptions it shouldn't. To improve code quality and precision, I've enabled the E722 flag. This will help make the code more maintainable and avoid potential issues. I'm committed to following best practices and making more contributions to improve the project. Let me know if you have any further concerns. |
|
Yeah, I was thinking it would be easier to replace them with |
|
Yes, with custom exception the review is really hard (it took me some hours to discover those exceptions). |
|
z@MarcoFalke @aureleoules new commit 61bb4e7 |
|
Thanks. I've read https://www.flake8rules.com/rules/E722.html and checked that the changes implement the description. lgtm ACK 61bb4e7 |
61bb4e7 lint: enable E722 do not use bare except (Leonardo Lazzaro) Pull request description: Improve test code and enable E722 lint check. If you want to catch all exceptions that signal program errors, use except Exception: (bare except is equivalent to except BaseException:). Reference: https://peps.python.org/pep-0008/#programming-recommendations ACKs for top commit: MarcoFalke: lgtm ACK 61bb4e7 Tree-SHA512: c7497769d5745fa02c78a20f4a0e555d8d3996d64af6faf1ce28e22ac1d8be415b98e967294679007b7bda2a9fd04031a9d140b24201e00257ceadeb5c5d7665
Improve test code and enable E722 lint check.
If you want to catch all exceptions that signal program errors, use except Exception: (bare except is equivalent to except BaseException:).
Reference: https://peps.python.org/pep-0008/#programming-recommendations