Skip to content

Conversation

@llazzaro
Copy link
Contributor

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

Copy link
Member

@luke-jr luke-jr left a 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.

@DrahtBot
Copy link
Contributor

DrahtBot commented Sep 23, 2022

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK MarcoFalke
Concept ACK luke-jr
Stale ACK aureleoules

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #27034 (rpc: make importaddress compatible with descriptors wallet by furszy)

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.

@llazzaro llazzaro changed the title lint: enable E722 do not use bare except DRAFT lint: enable E722 do not use bare except Oct 10, 2022
@llazzaro llazzaro force-pushed the master branch 2 times, most recently from 9b398c0 to 0b4f624 Compare October 10, 2022 20:46
@llazzaro llazzaro changed the title DRAFT lint: enable E722 do not use bare except lint: enable E722 do not use bare except Oct 10, 2022
@llazzaro
Copy link
Contributor Author

@luke-jr @aureleoules
The PR is ready.
Changed to specific exceptions except on the following cases:

test/functional/test_framework/p2p.py +383 code is very dynamic.
test/util/test_runner.py +54 calls bctest which raises Exception

Thanks in advance!

Copy link
Contributor

@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0b4f62415a4d141670de119d479fdebbd16a054f - LGTM

@fanquake
Copy link
Member

@MarcoFalke merge or close?

@maflcko
Copy link
Member

maflcko commented Feb 16, 2023

bare excepts should be replaced with specific exceptions, not except Exception.

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.

@llazzaro
Copy link
Contributor Author

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.

@maflcko
Copy link
Member

maflcko commented Feb 17, 2023

Yeah, I was thinking it would be easier to replace them with except Exception, like you did initially. This would mean the review is easy, because it can only affect KeyError, right? With several exceptions listed, we'd have to review the whole call stack, which is burdensome, no?

@llazzaro
Copy link
Contributor Author

Yes, with custom exception the review is really hard (it took me some hours to discover those exceptions).
If you agree I can switch back to Exception, the change is more simple and less error prone.

@llazzaro
Copy link
Contributor Author

z@MarcoFalke @aureleoules new commit 61bb4e7

@maflcko
Copy link
Member

maflcko commented Feb 21, 2023

Thanks. I've read https://www.flake8rules.com/rules/E722.html and checked that the changes implement the description.

lgtm ACK 61bb4e7

@DrahtBot DrahtBot requested a review from aureleoules February 21, 2023 15:07
@fanquake fanquake merged commit 0c57920 into bitcoin:master Feb 22, 2023
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Feb 25, 2023
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
@bitcoin bitcoin locked and limited conversation to collaborators Feb 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants