Python modernization 2 #471

jasonrhaas · 2017-11-27T20:05:12Z

What was wrong?

Finishing remaining items on #339.

How was it fixed?

Deleted old code.

Cute Animal Picture

hippo

techtonik · 2017-11-27T20:29:34Z

web3/account.py

        return self.privateKeyToAccount(key_bytes)

    @staticmethod
    def decrypt(keyfile_json, password):
-        if isinstance(keyfile_json, str) or (
-                sys.version_info.major < 3 and isinstance(keyfile_json, unicode)):  # noqa: 821
+        if isinstance(keyfile_json, str):  # noqa: 821


Why noqa here?

@techtonik that was already in the code so I just left it there. I'm not sure what the comment is referencing.

Just FYI, these noqa comments are a way of telling flake8 not to issue the specified warning. Most of the time it's best to fix the thing causing the warning, but occasionally the best thing to do is suppress it.

The list of warnings are here: http://flake8.pycqa.org/en/3.4.1/user/error-codes.html#error-violation-codes

821 is usage of an undefined name, which is usually pretty bad, but in this case it was just because unicode isn't defined in py3, and we know a py3 codepath to that unicode in this line is impossible.

Now that unicode is removed, we can and should remove the warning supressor.

techtonik · 2017-11-27T20:30:47Z

web3/account.py

@@ -36,8 +35,7 @@
    hexstr_if_str,
    text_if_str,
    to_bytes,
-    to_int,
-    to_hex,


techtonik · 2017-11-27T20:32:18Z

web3/utils/encoding.py

-        In Python 2, only a unicode object will be interpreted as unicode text
-        In Python 3, only a str object will be interpreted as interpreted as unicode text
+    @param hexstr_or_primitive in bytes, str, or int.
+        In Python 3, only a str object will be interpreted as unicode text


It is all Python 3 now.

I guess its safe to delete all the "python 3" comments?

techtonik · 2017-11-27T20:37:07Z

web3/utils/encoding.py

@@ -194,7 +193,6 @@ def to_bytes(primitive=None, hexstr=None, text=None):
    elif is_integer(primitive):
        return to_bytes(hexstr=to_hex(primitive))
    elif hexstr is not None:
-        hexstr = hexstr.rstrip('L')  # handle longs in Python 2


See also line 152 https://github.com/ethereum/web3.py/pull/471/files#diff-0260bdf984abdd4b8ab8987ea75d032cR152

jasonrhaas · 2017-11-27T23:40:54Z

Its failing in Travis on some weird ValueError: embedded null byte error. It doesn't happen on my local docker image, I'm assuming its something with the Linux Travis environment?

self = environ({'LANG': 'en_US.UTF-8', 'GOPATH': '/home/travis/gopath', 'GOROOT': '/home/travis/.gimme/versions/go1.7.4.linux...:/opt/pyenv/bin', 'PYTHONHASHSEED': '3431167186', 'VIRTUAL_ENV': '/home/travis/build/ethereum/web3.py/.tox/py36-core'})

key = b'PYTEST_CURRENT_TEST'

value = b'tests/core/eth-module/test_accounts.py::test_eth_account_hash_message_text[instance-Hello World-\\xa1\\xde\\x98\\x86\x00\\xa4,KJ\\xb0\\x89\\xb6\x19)|\x17\\xd5<\\xff\\xae]Q \\xd8-\\x8a\\x92\\xd0\\xbb;x\\xf2] (setup)'

    def __setitem__(self, key, value):

        key = self.encodekey(key)

        value = self.encodevalue(value)

>       self.putenv(key, value)

E       ValueError: embedded null byte

pipermerriam · 2017-11-27T23:45:24Z

Looks like a pytest bug. The test name is being derived from the fixture values and the fixture has null bytes in it. Looks similar to pytest-dev/pytest#2644

pipermerriam · 2017-11-27T23:47:12Z

@jasonrhaas work-around would be to to explicitely name the tests using one of these methods:

https://docs.pytest.org/en/latest/example/parametrize.html#different-options-for-test-ids

pipermerriam · 2017-11-27T23:48:15Z

pytest-dev/pytest#2649 looks related.

pytest-dev/pytest#2644

Used a list comprehension for `ids=` to work around this.

jasonrhaas · 2017-11-28T03:05:51Z

@pipermerriam I worked around this issue by assigning ids to all of the @paramatrize() statements. In most cases I used something like ids=['test_' + str(x) for x in range(10)] to give it a list of strings. Slightly hacky, but it fixes the problem.

carver · 2017-11-28T23:05:14Z

Hey, I've been reviewing and merging from oldest submissions to newest, and it looks like #456 did a lot of these same changes (hence the conflicts). I just took a pass at the conflicts, and will do a final review before merging...

carver · 2017-11-28T23:11:37Z

Pinning pytest to 3.2.5 also does the trick, but specifying ids also make the test results easier to read so I'm happy to see those in place too.

carver

Adding the ids is nice in some places (adds semantic value to the name of the test, like test_sig_bytes_chain_naive_v).

In other places, it just obscures the test meaning, when it would be useful to see the names generated by inputs.

Now that the pinned pytest version doesn't have the null byte problem, let's keep the ids that add semantic value, and remove the ones that obscure it.

carver · 2017-11-28T23:13:38Z

tests/core/utilities/test_abi_is_encodable.py

@@ -56,6 +56,7 @@
        ('0xname.eth', 'address', True),  # 0x in name is fine, if it is not a TLD
        ('0xff', 'address', False),  # but any valid hex starting with 0x should be rejected
    ),
+    ids=['test_' + str(x) for x in range(42)]


Since these ids actually obscure the test results, let's just remove them now that pytest is pinned to 3.2.5.

carver · 2017-11-28T23:14:22Z

tests/core/web3-module/test_conversions.py

@@ -18,7 +18,8 @@
        (256, b'\x01\x00'),
        (True, b'\x01'),
        (False, b'\x00'),
-    )
+    ),
+    ids=['test_' + str(x) for x in range(10)]


The ids in this module can go too (although the trailing comma on the list of args can stay).

carver · 2017-11-28T23:14:51Z

tests/core/web3-module/test_sha3.py

@@ -11,7 +11,8 @@
    [
        ('cowmö', HexBytes('0x0f355f04c0a06eebac1d219b34c598f85a1169badee164be8a30345944885fe8')),
        ('', HexBytes('0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470')),
-    ]
+    ],
+    ids=['test_' + str(x) for x in range(2)]


These ids can go too (although the trailing comma on the list of args can stay).

jasonrhaas · 2017-11-29T06:07:00Z

@carver this should be ready to merge once the Travis Tests finish running.

jasonrhaas added 2 commits November 27, 2017 14:51

Remove all sys.version_info and unicode checks

4fadc90

Remove a couple more Python 2 relics

b8dd981

techtonik reviewed Nov 27, 2017

View reviewed changes

web3/account.py Outdated

@@ -36,8 +35,7 @@

hexstr_if_str,

text_if_str,

to_bytes,

to_int,

to_hex,

Copy link

Contributor

techtonik Nov 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

techtonik reviewed Nov 27, 2017

View reviewed changes

jasonrhaas added 3 commits November 27, 2017 16:43

Flake8 fixes

71feb6c

Remove python 2 r.strip('L') statement

78ee0f4

Remove more Python 3 version checks

77e6df5

jasonrhaas added 2 commits November 27, 2017 19:19

Add explicit parametrize ids to work around pytest bug

0d43fa4

pytest-dev/pytest#2644

Add explicit test ids for other parametrize tests that were failing

3b9eb40

Used a list comprehension for `ids=` to work around this.

jasonrhaas changed the title ~~WIP: Python moderization 2~~ Python moderization 2 Nov 28, 2017

jasonrhaas changed the title ~~Python moderization 2~~ Python modernization 2 Nov 28, 2017

Merge branch 'master' into python-moderization-2

deaf5bd

Merge branch 'master' into python-moderization-2

4958b1a

carver requested changes Nov 28, 2017

View reviewed changes

Roll back some ids as per @carver request

4c52f79

carver approved these changes Nov 29, 2017

View reviewed changes

carver merged commit 29085d0 into ethereum:master Nov 29, 2017

carver mentioned this pull request Nov 29, 2017

Python Modernization #339

Closed

6 tasks

jasonrhaas deleted the python-moderization-2 branch November 29, 2017 18:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Python modernization 2 #471

Python modernization 2 #471

jasonrhaas commented Nov 27, 2017

techtonik Nov 27, 2017

jasonrhaas Nov 27, 2017

carver Nov 28, 2017

techtonik Nov 27, 2017

techtonik Nov 27, 2017

jasonrhaas Nov 27, 2017

carver Nov 28, 2017

techtonik Nov 27, 2017

jasonrhaas Nov 27, 2017

jasonrhaas commented Nov 27, 2017 •

edited

Loading

pipermerriam commented Nov 27, 2017

pipermerriam commented Nov 27, 2017

pipermerriam commented Nov 27, 2017

jasonrhaas commented Nov 28, 2017 •

edited

Loading

carver commented Nov 28, 2017

carver commented Nov 28, 2017

carver left a comment

carver Nov 28, 2017

carver Nov 28, 2017

carver Nov 28, 2017

jasonrhaas commented Nov 29, 2017

Python modernization 2 #471

Python modernization 2 #471

Conversation

jasonrhaas commented Nov 27, 2017

What was wrong?

How was it fixed?

Cute Animal Picture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrhaas commented Nov 27, 2017 • edited Loading

pipermerriam commented Nov 27, 2017

pipermerriam commented Nov 27, 2017

pipermerriam commented Nov 27, 2017

jasonrhaas commented Nov 28, 2017 • edited Loading

carver commented Nov 28, 2017

carver commented Nov 28, 2017

carver left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonrhaas commented Nov 29, 2017

jasonrhaas commented Nov 27, 2017 •

edited

Loading

jasonrhaas commented Nov 28, 2017 •

edited

Loading