Remove wildcard imports from qa #9876

1. 
   jnewbery commented at 2:15 pm on February 27, 2017: member

   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.

2. Change authproxy.py to be Python3 and tidy up imports. 9f16c52bda
3. Eliminate wildcard imports from test_framework

   This commit eliminates wildcard imports from test_framework and tidies
   up the import order (standard library first, then package modules, both
   sorted in alphabetical order).

   14e92d866e
4. Tidy up imports

   This commit tidies the import statements at the top of the individual
   test modules. Imports are grouped by:
   
   - standard library imports
   - third party imports
   - local package imports
   
   the groups are separated by a line and arranged in alphabetical order.

   506e23280d
5. Remove wildcard imports

   Wildcard imports (`from foo import *`) are generally considered bad
   practice in Python. They make it unclear which names are present in
   the namespace, which is confusing for both human readers and static
   analysis tools.
   
   This commit removes wildcard imports from all the test scripts and
   replaces them with explicit imports.

   f287e8de1e
6. 
   jnewbery commented at 2:26 pm on February 27, 2017: member

   I’ve run the extended tests locally and they all pass.
7. MarcoFalke added the label Tests on Feb 27, 2017
8. in qa/rpc-tests/p2p-segwit.py: in f287e8de1e outdated

   53+                                     msg_witness_block,
   54+                                     msg_witness_tx,
   55+                                     ser_uint256,
   56+                                     ser_vector,
   57+                                     sha256,
   58+                                     uint256_from_str)
   

   ---

   ---

   

   ryanofsky commented at 6:04 pm on February 27, 2017:

   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

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

   with column_limit=119.

   ---

   

   laanwj commented at 10:55 am on February 28, 2017:

   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 commented at 4:14 pm on February 28, 2017:

   I prefer the way @ryanofsky’s formatting looks, but I kept the imports as one-item-per-line for the reason @laanwj gives.
9. 
   ryanofsky commented at 6:09 pm on February 27, 2017: member

   utACK f287e8de1edf9a0b342b3e5b98b88e7bb1a2bd47
10. 
    laanwj commented at 10:55 am on February 28, 2017: member

    Concept ACK.
11. 
    sdaftuar commented at 2:43 pm on February 28, 2017: member

    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.

12. 
    jnewbery commented at 4:26 pm on February 28, 2017: member

    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.

13. nits. To squash 329f0c631b
14. jnewbery force-pushed on Feb 28, 2017
15. 
    JeremyRubin commented at 9:27 pm on February 28, 2017: contributor

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

     0# Current master
     1import * from math
     2
     3# jnewbery current proposal
     4from math import (sin,
     5                  cos,
     6                  pow)
     7
     8# my proposal
     9from math import sin
    10from math import cos
    11from math import pow
    

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

16. 
    jnewbery commented at 9:41 pm on February 28, 2017: member

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

17. 
    MarcoFalke commented at 11:46 pm on February 28, 2017: member

    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.
18. 
    jnewbery commented at 4:19 am on March 1, 2017: member

    @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.
19. 
    laanwj commented at 9:06 am on March 1, 2017: member

    @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.
20. 
    jtimon commented at 5:29 pm on March 7, 2017: contributor

    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.

21. 
    jnewbery commented at 6:58 pm on March 7, 2017: member

    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.
22. 
    MarcoFalke commented at 7:26 pm on March 7, 2017: member

    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.

23. 
    jnewbery commented at 7:31 pm on March 7, 2017: member

    ok, I’ll break this down into smaller chunks, and hopefully nudge the test_framework directory in the right direction.
24. jnewbery closed this on Mar 7, 2017

    ---

25. 
    laanwj commented at 6:32 am on March 8, 2017: member

    Hope to see “increases test coverage by XX%” for the next test-related pull instead :)
26. MarcoFalke locked this on Sep 8, 2021

Contributors
jnewbery ryanofsky laanwj sdaftuar JeremyRubin MarcoFalke jtimon

Labels
Tests