lint: enable E722 do not use bare except

llazzaro commented at 6:30 pm on August 18, 2022: 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

luke-jr changes_requested

luke-jr commented at 1:11 am on August 19, 2022: member

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

glozow added the label Scripts and tools on Aug 19, 2022

llazzaro force-pushed on Aug 30, 2022

llazzaro force-pushed on Sep 13, 2022

DrahtBot commented at 9:40 am on September 23, 2022: contributor

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 force-pushed on Oct 8, 2022

in test/functional/rpc_preciousblock.py:18 in 9467bcf4c9 outdated

15@@ -15,8 +16,7 @@ def unidirectional_node_sync_via_rpc(node_src, node_dest):
16     while True:
17         try:
18             assert len(node_dest.getblock(blockhash, False)) > 0
19-            break

aureleoules commented at 8:45 am on October 10, 2022:

this break should not be removed

llazzaro commented at 7:49 pm on October 10, 2022:

Yes, that’s why the CI timeout. I will resolve once I move the PR to ready

llazzaro commented at 7:56 pm on October 10, 2022:

Reverted break removed by mistake

in test/functional/feature_dbcrash.py:89 in 9467bcf4c9 outdated

84@@ -85,7 +85,7 @@ def restart_node(self, node_index, expected_tip):
85                 self.nodes[node_index].waitforblock(expected_tip)
86                 utxo_hash = self.nodes[node_index].gettxoutsetinfo()['hash_serialized_2']
87                 return utxo_hash
88-            except:
89+            except (JSONRPCException, ConnectionResetError, OSError, ValueError):

aureleoules commented at 8:46 am on October 10, 2022:

missing from test_framework.authproxy import JSONRPCException in feature_dbcrash.py

llazzaro commented at 7:49 pm on October 10, 2022:

Please note that the PR is not ready yet. Thanks for the review

llazzaro commented at 7:57 pm on October 10, 2022:

added missing import

aureleoules changes_requested

llazzaro renamed this:
~~lint: enable E722 do not use bare except~~
DRAFT lint: enable E722 do not use bare except
on Oct 10, 2022

llazzaro force-pushed on Oct 10, 2022

llazzaro renamed this:
~~DRAFT lint: enable E722 do not use bare except~~
lint: enable E722 do not use bare except
on Oct 10, 2022

llazzaro commented at 9:36 pm on October 10, 2022: contributor

@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!

aureleoules approved

aureleoules commented at 4:53 pm on October 11, 2022: member

ACK 0b4f62415a4d141670de119d479fdebbd16a054f - LGTM

fanquake commented at 12:35 pm on February 16, 2023: member

@MarcoFalke merge or close?

maflcko commented at 4:15 pm on February 16, 2023: member

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 commented at 1:00 pm on February 17, 2023: contributor

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 commented at 4:04 pm on February 17, 2023: member

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 commented at 5:25 pm on February 17, 2023: contributor

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.

lint: enable E722 do not use bare except 61bb4e783b

llazzaro force-pushed on Feb 18, 2023

llazzaro commented at 10:07 pm on February 18, 2023: contributor

z@MarcoFalke @aureleoules new commit 61bb4e7

maflcko commented at 3:07 pm on February 21, 2023: member

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

lgtm ACK 61bb4e783b3acc62b121a228f6b14c2462e23315

DrahtBot requested review from aureleoules on Feb 21, 2023

fanquake merged this on Feb 22, 2023

fanquake closed this on Feb 22, 2023

sidhujag referenced this in commit ef67c35ab1 on Feb 25, 2023

bitcoin locked this on Feb 22, 2024

lint: enable E722 do not use bare except #25867

Reviews

Conflicts