trezor / python-trezor

:snake: Don't use this repo, use the new monorepo instead:
https://github.com/trezor/trezor-firmware
GNU Lesser General Public License v3.0
201 stars 194 forks source link

Replace hexlify/unhexlify with hex/fromhex #309

Closed prusnak closed 5 years ago

prusnak commented 6 years ago

Now that we have proper (un)hexlify methods, let's just use that. It increases readability, especially of the tests.

i.e. replace all usage of binascii.hexlify(foo) with foo.hex() and all usage of binascii.unhexlify(bar) with bytes.fromhex(bar)

That is if we want to drop compatibility with Python 3.4 and older.

prusnak commented 6 years ago

I went ahead and did the mentioned changes in all files in the tests/ directory: see the kill_binascii branch

I think we can drop the compatibility with older Python in tests (both unit tests and device tests).

Not sure about tools, trezorctl and client, though. :-/

prusnak commented 6 years ago

In 994d1c5ba9936eee9bdc6bdcb1976e0dffe22ba5 of the kill_binascii branch, I have modified tests. I have run the new tests and they work, thus this can be merged IMO.

In 8d5f693cf719c8e2f9f7037261e663cd9063c159 of the same branch, I have modified trezorlib+tools. This I have not tested properly and also requires special consideration, whether we want to introduce the dependency on Python 3.5. For reference, Electrum now requires Python 3.6, so I think we might not lose too much, anyway.

matejcik commented 6 years ago

ISTM dropping it from tests and not from the main lib doesn't bring us much. (after all the device test suite can be used to test trezorlib itself, although Travis currently doesn't do that), so I'd do both or neither.

(As an aside, it would be more interesting to go to 3.6, but that's probably a bad idea today.)

Oldest LTS Debian has python 3.4 and lives until 2020, but current stable has 3.5. Oldest LTS Ubuntu also has python 3.4 and lives until 2019, but more up-to-date versions have 3.5. It's probably safe to freeze them and other long-lived LTS distros on the 0.10.x branch.

How about this: I'd let this bug sit until I return from vacation, and if nobody comes up with a good reason to keep py3.4 support, we'll merge it?

prusnak commented 6 years ago

OK

matejcik commented 5 years ago

merged into master, binascii is gone and we now only support python 3.5 and up