Python cleanups by practicalswift · Pull Request #10781 · bitcoin/bitcoin

practicalswift · 2017-07-09T21:16:20Z

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

jnewbery

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?

jnewbery · 2017-07-10T13:15:34Z

test/functional/p2p-compactblocks.py

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

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?

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.

jtimon · 2017-07-13T20:27:30Z

test/functional/bumpfee.py

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

jtimon · 2017-07-13T20:28:23Z

fast review ACK

practicalswift · 2017-07-16T11:46:04Z

@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 · 2017-07-20T14:27:38Z

Added two commits :-)

jnewbery · 2017-07-20T20:20:00Z

test/functional/test_framework/socks5.py

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.

jnewbery · 2017-07-20T20:22:47Z

test/functional/preciousblock.py

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

jnewbery · 2017-07-20T20:29:30Z

test/functional/test_framework/netutil.py

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

Good point! Fixed!

jnewbery · 2017-07-20T20:29:57Z

test/functional/test_framework/mininode.py

No harm in leaving these as comments.

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

jnewbery · 2017-07-20T20:33:21Z

test/functional/test_framework/key.py

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

jnewbery · 2017-07-20T20:34:00Z

test/functional/test_framework/authproxy.py

Please leave this for now.

jnewbery · 2017-07-20T20:36:54Z

Mostly ACK, but a few comments.

practicalswift · 2017-07-20T21:25:24Z

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

jnewbery · 2017-07-20T21:49:33Z

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

practicalswift · 2017-07-20T21:56:46Z

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

jnewbery · 2017-07-20T21:58:14Z

looks good. utACK db442ada2b7e162c3748ddcc130c414fd86e2bab

…in programs

…does

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

practicalswift · 2017-08-28T13:19:53Z

Rebased and and added another commit ("Remove import of unused constants").

@jnewbery, would you mind re-reviewing? :-)

jnewbery · 2017-08-28T14:04:25Z

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 · 2017-08-28T14:54:36Z

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

jtimon · 2017-08-28T15:11:17Z

repeatead fast review ack

meshcollider · 2017-08-28T19:58:06Z

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

practicalswift force-pushed the python-cleanups branch 2 times, most recently from 2190ff6 to a2724e4 Compare July 9, 2017 21:36

fanquake added the Refactoring label Jul 10, 2017

jnewbery reviewed Jul 10, 2017

View reviewed changes

jtimon reviewed Jul 13, 2017

View reviewed changes

test/functional/bumpfee.py Outdated

Copy link

Contributor

jtimon Jul 13, 2017

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?

practicalswift force-pushed the python-cleanups branch from a2724e4 to 52ae217 Compare July 16, 2017 11:42

practicalswift mentioned this pull request Jul 19, 2017

trivial: fix various pyflakes/vulture warnings #10881

Closed

jnewbery reviewed Jul 20, 2017

View reviewed changes

test/functional/test_framework/authproxy.py Outdated

Copy link

Contributor

jnewbery Jul 20, 2017

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.

practicalswift force-pushed the python-cleanups branch 2 times, most recently from 441552d to 8693042 Compare July 20, 2017 21:22

practicalswift force-pushed the python-cleanups branch from 8693042 to db442ad Compare July 20, 2017 21:55

practicalswift added 6 commits August 28, 2017 15:18

Use sys.exit(...) instead of exit(...): exit(...) should not be used …

25cd520

…in programs

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

51cb6b8

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

9b94054

…does

Remove unused variables and/or function calls

2e6080b

Use the variable name _ for unused return values

8239794

Use for-loop instead of list comprehension

7821458

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

practicalswift force-pushed the python-cleanups branch from db442ad to 9322dad Compare August 28, 2017 13:19

practicalswift force-pushed the python-cleanups branch from 9322dad to 7821458 Compare August 28, 2017 14:54

maflcko merged commit 7821458 into bitcoin:master Aug 28, 2017

practicalswift deleted the python-cleanups branch April 10, 2021 19:32

barton2526 mentioned this pull request Aug 4, 2021

refactor: small python cleanup gridcoin-community/Gridcoin-Research#2267

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 18, 2022

Conversation

practicalswift commented Jul 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jul 13, 2017

Uh oh!

practicalswift commented Jul 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Jul 20, 2017

Uh oh!

jnewbery Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

practicalswift Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jul 20, 2017

Uh oh!

practicalswift commented Jul 20, 2017

Uh oh!

jnewbery commented Jul 20, 2017

Uh oh!

practicalswift commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Jul 20, 2017

Uh oh!

practicalswift commented Aug 28, 2017

Uh oh!

jnewbery commented Aug 28, 2017

Uh oh!

practicalswift commented Aug 28, 2017

Uh oh!

jtimon commented Aug 28, 2017

Uh oh!

meshcollider commented Aug 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

practicalswift commented Jul 9, 2017 •

edited

Loading

practicalswift commented Jul 16, 2017 •

edited

Loading

jnewbery Jul 20, 2017 •

edited

Loading

practicalswift Jul 20, 2017 •

edited

Loading

practicalswift commented Jul 20, 2017 •

edited

Loading