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 cleanups #10781
Python cleanups #10781
Conversation
2190ff6
to
a2724e4
Compare
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.
Looks good. utACK. One question inline and another general question:
In commit Use the variable name _ to show that we are intentionally ignoring a return value, why not just ignore the return value?
@@ -293,7 +293,7 @@ def test_compactblock_construction(self, node, test_node, version, use_witness_a | |||
|
|||
# Store the raw block in our internal format. | |||
block = FromHex(CBlock(), node.getblock("%02x" % block_hash, False)) | |||
[tx.calc_sha256() for tx in block.vtx] |
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.
I can't see how using tx
is an issue. Does this cause a linter warning?
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.
Yes it does :-)
$ flake8 test/functional/p2p-compactblocks.py | grep redefine
test/functional/p2p-compactblocks.py:296:31: F812 list comprehension redefines 'tx' from line 276
Should I remove the commit 263686d03fbfa2191c160d4c50951852fa1a4900 from this PR?
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.
actually, let's just swap this list comprehension for a for loop. We're using it for its side-effects, so a for loop is clearer.
test/functional/bumpfee.py
Outdated
@@ -168,7 +168,7 @@ def test_bumpfee_with_descendant_fails(rbf_node, rbf_node_address, dest_address) | |||
parent_id = spend_one_input(rbf_node, rbf_node_address) | |||
tx = rbf_node.createrawtransaction([{"txid": parent_id, "vout": 0}], {dest_address: 0.00020000}) | |||
tx = rbf_node.signrawtransaction(tx) | |||
txid = rbf_node.sendrawtransaction(tx["hex"]) | |||
_ = rbf_node.sendrawtransaction(tx["hex"]) |
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.
what about something more clear like throwaway_txid or something of the sort?
fast review ACK |
a2724e4
to
52ae217
Compare
Added two commits :-) |
@@ -91,7 +91,7 @@ def handle(self): | |||
self.conn.sendall(bytearray([0x01, 0x00])) | |||
|
|||
# Read connect request | |||
(ver,cmd,rsv,atyp) = recvall(self.conn, 4) | |||
(ver, cmd, _, atyp) = recvall(self.conn, 4) |
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.
no need for this to be a tuple. Just:
ver, cmd, _, atyp = recvall(self.conn, 4)
There are other examples in commit Use the variable name _ for unused return values. I won't comment on each of them.
test/functional/preciousblock.py
Outdated
@@ -47,16 +47,16 @@ def run_test(self): | |||
self.log.info("Ensure submitblock can in principle reorg to a competing chain") | |||
self.nodes[0].generate(1) | |||
assert_equal(self.nodes[0].getblockcount(), 1) | |||
(hashY, hashZ) = self.nodes[1].generate(2) | |||
(_, hashZ) = self.nodes[1].generate(2) |
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.
prefer:
hashZ = self.nodes[1].generate(2)[-1]
since it conveys the meaning better: "give me the hash of the last block generated".
same for the two examples below
@@ -15,17 +15,7 @@ | |||
import os | |||
from binascii import unhexlify, hexlify | |||
|
|||
STATE_ESTABLISHED = '01' |
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.
Perhaps change these to comments rather than erase them (for context).
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.
Good point! Fixed!
@@ -48,8 +48,6 @@ | |||
COIN = 100000000 # 1 btc in satoshis | |||
|
|||
NODE_NETWORK = (1 << 0) | |||
NODE_GETUTXO = (1 << 1) |
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.
No harm in leaving these as comments.
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.
Good point! Updated but GitHub doesn't show the # NODE_GETUTXO = (1 << 1)
below (in the comment view), so it appears as if it is not fixed :-)
@@ -112,40 +112,16 @@ def set_secretbytes(self, secret): | |||
ssl.BN_CTX_free(ctx) | |||
return self.k | |||
|
|||
def set_privkey(self, key): |
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.
I can imagine these methods potentially being useful at some point. I don't think there's any need to remove them.
@@ -158,11 +158,6 @@ def __call__(self, *args, **argsn): | |||
else: | |||
return response['result'] | |||
|
|||
def _batch(self, rpc_call_list): |
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.
Please leave this for now.
Mostly ACK, but a few comments. |
441552d
to
8693042
Compare
@jnewbery Thanks for reviewing! Good feedback - I've addressed all points raised. Would you mind re-reviewing? :-) |
Looks good. Just #10781 (comment) unaddressed (using a for loop instead of list comprehension for functions with side-effects) |
8693042
to
db442ad
Compare
@jnewbery Thanks! List comprehension now fixed as well. Looks good? :-) |
looks good. utACK db442ada2b7e162c3748ddcc130c414fd86e2bab |
To make it clear that we are intentionally using it for its side-effects.
db442ad
to
9322dad
Compare
Rebased and and added another commit ("Remove import of unused constants"). @jnewbery, would you mind re-reviewing? :-) |
Looks good to me, but can you remove the final commit? Marco's #11067 will already remove those unused constants. @MarcoFalke - if you have time, can you check whether this is suitable for merge? It's likely to require continual rebase if left open. I've verified that @practicalswift hasn't added any backdoors in the last rebase :) |
9322dad
to
7821458
Compare
@jnewbery Removed last commit :-) Thanks fo reviewing :-) |
repeatead fast review ack |
Fast review utACK |
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
7821458 Use for-loop instead of list comprehension (practicalswift) 8239794 Use the variable name _ for unused return values (practicalswift) 2e6080b Remove unused variables and/or function calls (practicalswift) 9b94054 Avoid reference to undefined name: stderr does not exist, sys.stderr does (practicalswift) 51cb6b8 Use print(...) instead of undefined printf(...) (practicalswift) 25cd520 Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs (practicalswift) Pull request description: Python cleanups: * Avoid reference to undefined name: `stderr` does not exist, `sys.stderr` does * Use `print(...)` instead of undefined `printf(...)` * Avoid redefinition of variable (`tx`) in list comprehension * Remove unused variables and/or function calls * Use `sys.exit(...)` instead of `exit(...)`: [`exit(...)` should not be used in programs](bitcoin#10753 (comment)) Tree-SHA512: 1238dfbc1d20f7edadea5e5406a589f293065638f6234809f0d5b6ba746dffe3d276bc5884c7af388a6c798c61a8759faaccf57f381225644754c0f61914eb4b
Python cleanups:
stderr
does not exist,sys.stderr
doesprint(...)
instead of undefinedprintf(...)
tx
) in list comprehensionsys.exit(...)
instead ofexit(...)
:exit(...)
should not be used in programs