Remove wildcard imports from qa

[jnewbery]

In Python, wildcard imports are generally discouraged for much the same reason that using namespace is generally discouraged in C++. Namely because it makes it unclear where names are coming from, and makes reading code more difficult for humans and tools.

This PR removes all wildcard imports from the qa directory and replaces them with explicit imports.

Concretely, I'm planning on doing some refactors of the test_framework library, and not being able to quickly see all of the dependencies makes that more difficult.

But wait, there's more: as a bonus I've tided up the imports so they're in PEP8 ordering: std library first, then third party packages, then local modules.

[jnewbery]

I've run the extended tests locally and they all pass.

[ryanofsky]

utACK f287e8d

[ryanofsky]

This is fine, but the import list is super long. Did you use a tool to generate this? Wondering if it would be difficult to just replace all this with from test_framework import mininode and then add mininode. qualifiers where needed.

Alternately, maybe different formatting would be preferable. I've been using yapf for new code I write, which gives

+from test_framework.script import (CScript, CScriptNum, CScriptOp, CTransaction, CTxOut, OP_0, OP_1, OP_16, OP_2DROP,
+                                   OP_CHECKMULTISIG, OP_CHECKSIG, OP_DROP, OP_DUP, OP_ELSE, OP_ENDIF, OP_EQUAL,
+                                   OP_EQUALVERIFY, OP_HASH160, OP_IF, OP_RETURN, OP_TRUE, SIGHASH_ALL,
+                                   SIGHASH_ANYONECANPAY, SIGHASH_NONE, SIGHASH_SINGLE, SegwitVersion1SignatureHash,
+                                   SignatureHash, hash160, hash256, ser_uint256, sha256, uint256_from_str)

with column_limit=119.

[laanwj]

One item per line is nice for diff-ing, though. Collapsing them can look somewhat nicer, however imagine having to insert an item in the middle and re-layouting. This will change all the lines, causing collisions with other patches.

For this reason we also use one-filename-per-line in makefiles, even though it results in long files.

[jnewbery]

I prefer the way @ryanofsky's formatting looks, but I kept the imports as one-item-per-line for the reason @laanwj gives.

[laanwj]

Concept ACK.

[sdaftuar]

I appreciate the desire for using best coding practices in the qa tree, and I certainly would like to improve my own test-writing habits. But I think we should be mindful of the tradeoffs of applying best practices for software that changes infrequently when applied to software that is written iteratively.

I tend to think of the test_framework/ directory as the former category; the test frameworks and utilities should not change often, except when we have a better idea of how to do our test writing, and then we should think carefully about the design changes we're making.

But I tend to think of the tests themselves as more iterative, because as we make changes to bitcoind, we often find ourselves needing to update existing tests. And I've personally found that even when I write new tests, I tend to write them iteratively, as I'll tend to test part of a PR, get it working, then continue to add more tests as I review more of a PR. And I think it'd be tedious to disallow wildcard imports for some of the contents in test_framework/, particularly the message types and data structures in mininode.py and the script op codes in script.py.

So I'd propose the following compromise: split the message types and script op codes out into their own files, get rid of all wildcard imports from test_framework/, but don't bother getting rid of wildcard imports from the new message type and script op code files.

On a related note, if we are going to start instituting style guidelines for the qa tree (which I do think is a good idea, whether we go with my proposal or not), we should write up what those are.

[jnewbery]

Thanks for taking the time to look at this @laanwj and @sdaftuar

So I'd propose the following compromise: split the message types and script op codes out into their own files

I'm currently working on a PR to move the message classes, bitcoin datastructure classes and serialization/deserialization functions into their own primitives.py module. I'll leave script op codes in scripts.py for now (unless you strongly believe they should also live in primitives).

get rid of all wildcard imports from test_framework/

done

don't bother getting rid of wildcard imports from the new message type and script op code files.

I'd still much prefer to remove wildcards and use something like import test_framework.script as bts and import test_framework.primitives as btp and then calling the script or primitive using the dotted notation.

[JeremyRubin]

I think I'd be in favor of making every import individually qualified (not sure right term). EG:

# Current master
import * from math

# jnewbery current proposal
from math import (sin,
                  cos,
                  pow)

# my proposal
from math import sin
from math import cos
from math import pow

Even though this has extra from math import comparatively, it makes it much easier to see what a diff is changing.

[jnewbery]

@JeremyRubin nice idea. I've never seen that style before, but I think I like it! I was a little concerned about whether python would execute the imported module multiple times, but I've done a quick test and it seems that it's smart enough to not do that.

(Yet) another option that might satisfy @sdaftuar is to explicitly index what names should be exportable from the test_framework package modules using the __all__ dunder. We could explicitly mark the bitcoin and script primitives as exportable and then use import * from script and import * from primitives in the test scripts without polluting our namespace.

[maflcko]

I like the suggestion by @jnewbery with using named imports as well as the __all__ syntax. Imo it is most appropriate to use the approach that makes most sense for each module.

E.g:

• Some util functions such as stop_node(# ,# ) should never be imported, but accessed through the framework instance. I.e. self.stop_node(i).
• Node specific methods and variables can be imported by "import as". I.e. import mininode as mn; mn.COIN;
• Plain utility functions such as double sha256 should be accessible without a named prefix, importable by wildcard or explicit import, whatever the author prefers.

[jnewbery]

@MarcoFalke - agree with everything you say, and it's what we should be aiming for. I think we should do the following:

• take this PR, which removes wildcards entirely
• I already have a branch ready which moves the primitives from mininode into their own module. I'll update that branch to include module-level all variables for primitives.py and script.py and make a PR for that. We can also update test cases which use many primitives and scripts (eg p2p-compactblocks.py) to use the wildcard imports. Authors of future testcases can choose the import style they prefer.
• util functions that shouldn't be exported from outside the package should be updated to have a leading underscore to mark them as private. This doesn't have any functional impact, but is a signal to users of the package that the function is private and shouldn't be imported.

[laanwj]

@sdaftuar Point taken. With software it is incredibly easy to spend countless hours on ancillary things that don't directly contribute to the goal in mind, sometimes to optimize arbitrary theoretical concerns. You write a lot of tests, so if you think this makes making new and better tests more work in practice, we should listen to that and take a different approach.

[jtimon]

Concept ACK

On a related note, if we are going to start instituting style guidelines for the qa tree, we should write up what those are.

Completely agree.
Needs rebase.

[jnewbery]

Thanks @jtimon

@MarcoFalke is it worth me rebasing this? I think it's the right direction to move in, but it seems like this PR might be a bit too much for people to take in one bite.

[maflcko]

The discussion above was indeed helpful to get a feeling about the direction we want to take. However, I don't think the pull in its current form is a net gain. As basically every import statement is changed, there will be numerous merge conflicts; And removing the wildcards as done currently is not helpful, considering that it makes writing tests harder. So I'd rather defer this for now.

[jnewbery]

ok, I'll break this down into smaller chunks, and hopefully nudge the test_framework directory in the right direction.

[laanwj]

Hope to see "increases test coverage by XX%" for the next test-related pull instead :)