Make script.py wildcard importable. #9943
Conversation
|
Needs a rebase. |
jnewbery
force-pushed
the
rpctestsprimitives
branch
from
5d64f54 to
105483c
Compare
jimmysong
left a comment
jimmysong
left a comment
There was a problem hiding this comment.
It looks like smartfees.py, bip9-softforks.py and bip65-cltv-p2p.py could also use the same import in test/functional. Also, address.py and blocktools.py could use the same import cleanup in test/functional/test_framework
test/functional/p2p-fullblocktest.py
Outdated
There was a problem hiding this comment.
It seems a little strange that hash160 is in script.py and hash256/sha256 are in mininode.py can we put all three in one place (probably mininode.py)? You're changing most of the places where this is being imported (smartfees.py and test_framework/address.py seem to be the only other ones). It might be worth just changing this, too.
There was a problem hiding this comment.
Yes, I agree that they should all live in the same place. My preference would be for utils.py to actually be pure utility functions and for the hash functions to live in there.
I haven't moved it in this commit because that seemed the minimal change. We should move hash160 to live alongside the other functions in a future PR that cleans up utils.py and mininode.py.
jnewbery
force-pushed
the
rpctestsprimitives
branch
from
105483c to
4114e1f
Compare
jnewbery
force-pushed
the
rpctestsprimitives
branch
from
4114e1f to
d5cc6ff
Compare
|
I didn't change the imports in smartfees.py, bip9-softforks.py and bip65-cltv-p2p.py because there's no harm in leaving them as explicit. I only moved segwit.py from exlicit to wildcard because it was importing an absurdly long list of names. for imports within the test_framework module itself, I think we should aim for them all to be explicit. I have updated address.py. It was previously importing hash256 and sha256 indirectly via script.py. I've now changed it so that it imports them directly from mininode.py. |
|
ACK d5cc6ff |
|
I'd love to tidy up imports in the test framework, but there doesn't seem to be general enthusiasm for this. Closing for now. |
3 participants
This PR is part of a larger effort to make the test_framework API more intuitive. See #9876 for background.
Several testcases use a wildcard import from test_framework.script. That's not ideal as it pollutes the namespace with all of the names defined in test_framework.script, including anything that script.py has imported itself.
This PR adds an
__all__module variable to script.py which indexes all of the names that should commonly be required by individual test cases. That makes the wildcard import a well-defined API and test cases can wildcard import without worrying about importing unexpected names. If a test case needs to import something from script.py which isn't included in__all__, it can explicitly import it.I've not included
hash160()orCScriptOp()in__all__becausehash160()is a helper function which I think should be defined elsewhere (ideally in the same module assha256(),ripemd160()andhash256()), andCScriptOpshould not commonly be required by testcases (the only place it's currently used is inGetP2PKHScript()in p2p-segwit.py, which arguably should be pulled up into the script.py module).