Python cleanups #10781

practicalswift commented at 9:16 PM on July 9, 2017: contributor

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

practicalswift force-pushed on Jul 9, 2017

fanquake added the label Refactoring on Jul 10, 2017

in test/functional/p2p-compactblocks.py:289 in a2724e4797 outdated

 292 | @@ -293,7 +293,7 @@ def test_compactblock_construction(self, node, test_node, version, use_witness_a
 293 |  
 294 |          # Store the raw block in our internal format.
 295 |          block = FromHex(CBlock(), node.getblock("%02x" % block_hash, False))
 296 | -        [tx.calc_sha256() for tx in block.vtx]

jnewbery commented at 1:15 PM on July 10, 2017:

I can't see how using tx is an issue. Does this cause a linter warning?

practicalswift commented at 11:22 AM on July 16, 2017:

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?

jnewbery commented at 8:12 PM on July 20, 2017:

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.

jnewbery commented at 1:20 PM on July 10, 2017: member

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?

in test/functional/bumpfee.py:171 in a2724e4797 outdated

 167 | @@ -168,7 +168,7 @@ def test_bumpfee_with_descendant_fails(rbf_node, rbf_node_address, dest_address)
 168 |      parent_id = spend_one_input(rbf_node, rbf_node_address)
 169 |      tx = rbf_node.createrawtransaction([{"txid": parent_id, "vout": 0}], {dest_address: 0.00020000})
 170 |      tx = rbf_node.signrawtransaction(tx)
 171 | -    txid = rbf_node.sendrawtransaction(tx["hex"])
 172 | +    _ = rbf_node.sendrawtransaction(tx["hex"])

jtimon commented at 8:27 PM on July 13, 2017:

what about something more clear like throwaway_txid or something of the sort?

jtimon commented at 8:28 PM on July 13, 2017: contributor

fast review ACK

practicalswift force-pushed on Jul 16, 2017

practicalswift commented at 11:46 AM on July 16, 2017: contributor

@jnewbery Good point! I've now changed from _ = f() to f() for calls that appear to have side effects. For calls that appear to have no side effects I've simply removed the calls. @jnewbery @jtimon Could you take a look at the updated version and see if it looks correct? :-)

practicalswift commented at 2:27 PM on July 20, 2017: contributor

Added two commits :-)

in test/functional/test_framework/socks5.py:94 in 0337632037 outdated

  90 | @@ -91,7 +91,7 @@ def handle(self):
  91 |                  self.conn.sendall(bytearray([0x01, 0x00]))
  92 |  
  93 |              # Read connect request
  94 | -            (ver,cmd,rsv,atyp) = recvall(self.conn, 4)
  95 | +            (ver, cmd, _, atyp) = recvall(self.conn, 4)

jnewbery commented at 8:20 PM on July 20, 2017:

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.

in test/functional/preciousblock.py:50 in 0337632037 outdated

  46 | @@ -47,16 +47,16 @@ def run_test(self):
  47 |          self.log.info("Ensure submitblock can in principle reorg to a competing chain")
  48 |          self.nodes[0].generate(1)
  49 |          assert_equal(self.nodes[0].getblockcount(), 1)
  50 | -        (hashY, hashZ) = self.nodes[1].generate(2)
  51 | +        (_, hashZ) = self.nodes[1].generate(2)

jnewbery commented at 8:22 PM on July 20, 2017:

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

in test/functional/test_framework/netutil.py:18 in 0337632037 outdated

  14 | @@ -15,17 +15,7 @@
  15 |  import os
  16 |  from binascii import unhexlify, hexlify
  17 |  
  18 | -STATE_ESTABLISHED = '01'

jnewbery commented at 8:29 PM on July 20, 2017:

Perhaps change these to comments rather than erase them (for context).

practicalswift commented at 9:16 PM on July 20, 2017:

Good point! Fixed!

in test/functional/test_framework/mininode.py:51 in 0337632037 outdated

  47 | @@ -48,8 +48,6 @@
  48 |  COIN = 100000000 # 1 btc in satoshis
  49 |  
  50 |  NODE_NETWORK = (1 << 0)
  51 | -NODE_GETUTXO = (1 << 1)

jnewbery commented at 8:29 PM on July 20, 2017:

No harm in leaving these as comments.

practicalswift commented at 9:15 PM on July 20, 2017:

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 :-)

in test/functional/test_framework/key.py:115 in 0337632037 outdated

 111 | @@ -112,40 +112,16 @@ def set_secretbytes(self, secret):
 112 |          ssl.BN_CTX_free(ctx)
 113 |          return self.k
 114 |  
 115 | -    def set_privkey(self, key):

jnewbery commented at 8:33 PM on July 20, 2017:

I can imagine these methods potentially being useful at some point. I don't think there's any need to remove them.

in test/functional/test_framework/authproxy.py:161 in 0337632037 outdated

 157 | @@ -158,11 +158,6 @@ def __call__(self, *args, **argsn):
 158 |          else:
 159 |              return response['result']
 160 |  
 161 | -    def _batch(self, rpc_call_list):

jnewbery commented at 8:34 PM on July 20, 2017:

Please leave this for now.

jnewbery commented at 8:36 PM on July 20, 2017: member

Mostly ACK, but a few comments.

practicalswift force-pushed on Jul 20, 2017

practicalswift commented at 9:25 PM on July 20, 2017: contributor

@jnewbery Thanks for reviewing! Good feedback - I've addressed all points raised. Would you mind re-reviewing? :-)

jnewbery commented at 9:49 PM on July 20, 2017: member

Looks good. Just #10781 (review) unaddressed (using a for loop instead of list comprehension for functions with side-effects)

practicalswift force-pushed on Jul 20, 2017

practicalswift commented at 9:56 PM on July 20, 2017: contributor

@jnewbery Thanks! List comprehension now fixed as well. Looks good? :-)

jnewbery commented at 9:58 PM on July 20, 2017: member

looks good. utACK db442ada2b7e162c3748ddcc130c414fd86e2bab

Use sys.exit(...) instead of exit(...): exit(...) should not be used in programs 25cd520fc4

Use print(...) instead of undefined printf(...) 51cb6b8221

Avoid reference to undefined name: stderr does not exist, sys.stderr does 9b94054b7c

Remove unused variables and/or function calls 2e6080bbf3

Use the variable name _ for unused return values 8239794360

Use for-loop instead of list comprehension

To make it clear that we are intentionally using it for its
side-effects.

78214588d6

practicalswift force-pushed on Aug 28, 2017

practicalswift commented at 1:19 PM on August 28, 2017: contributor

Rebased and and added another commit ("Remove import of unused constants"). @jnewbery, would you mind re-reviewing? :-)

jnewbery commented at 2:04 PM on August 28, 2017: member

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 :)

practicalswift force-pushed on Aug 28, 2017

practicalswift commented at 2:54 PM on August 28, 2017: contributor

@jnewbery Removed last commit :-) Thanks fo reviewing :-)

jtimon commented at 3:11 PM on August 28, 2017: contributor

repeatead fast review ack

meshcollider commented at 7:58 PM on August 28, 2017: contributor

Fast review utACK

MarcoFalke merged this on Aug 28, 2017

MarcoFalke closed this on Aug 28, 2017

MarcoFalke referenced this in commit 60dd9cc470 on Aug 28, 2017

PastaPastaPasta referenced this in commit 062561b702 on Sep 19, 2019

PastaPastaPasta referenced this in commit 9b09ca6f7d on Sep 23, 2019

PastaPastaPasta referenced this in commit 3375f6a77c on Sep 24, 2019

PastaPastaPasta referenced this in commit 94d1d28c54 on Nov 19, 2019

PastaPastaPasta referenced this in commit 3e9d3cc88f on Nov 21, 2019

PastaPastaPasta referenced this in commit a2bcdb9d1d on Dec 9, 2019

PastaPastaPasta referenced this in commit f64887dfe5 on Jan 1, 2020

PastaPastaPasta referenced this in commit bd89676e44 on Jan 2, 2020

PastaPastaPasta referenced this in commit a0a3d17e9a on Jan 2, 2020

PastaPastaPasta referenced this in commit 2c7c504b42 on Jan 2, 2020

PastaPastaPasta referenced this in commit dd89d1844b on Jan 2, 2020

PastaPastaPasta referenced this in commit c7aa2d6a6f on Jan 2, 2020

PastaPastaPasta referenced this in commit 561ec27683 on Jan 3, 2020

ckti referenced this in commit 5ac21eb8cb on Mar 28, 2021

practicalswift deleted the branch on Apr 10, 2021

gades referenced this in commit 339f96b813 on Jun 24, 2021

gades referenced this in commit 48c34380f5 on Feb 14, 2022

DrahtBot locked this on Aug 18, 2022