Skip to content

Remove attempted Python 2 compatibility - #247

Merged
petertodd merged 2 commits into
petertodd:masterfrom
ysangkok:remove-py2-compat
Nov 16, 2021
Merged

petertodd merged 2 commits into
petertodd:masterfrom
ysangkok:remove-py2-compat

Conversation

@ysangkok

Copy link
Copy Markdown
Contributor

This hacky approach precludes Mypy type checking.

Python 2 is not supported any more, and Tox is not running
the test suite in this version either.

@ysangkok ysangkok mentioned this pull request Oct 13, 2020
This hacky approach precludes Mypy type checking.

Python 2 is not supported any more, and Tox is not running
the test suite in this version either.
Comment thread bitcoin/rpc.py Outdated
@dgpv

dgpv commented Oct 13, 2020

Copy link
Copy Markdown
Contributor

I've read the diff, and the changes look good to me (except one small moment re hexlify/unhexlify with altered semantics, on which I commented on the diff itself)

@ysangkok

Copy link
Copy Markdown
Contributor Author

@dgpv Thanks for the quick review. I have added another commit that renames and documents the hexlify/unhexlify wrappers.

These wrapper work differently than functions from binascii,
even though they have the same name. That could be a source
of confusion. In this commit, the functions are renamed to
make it clear that they operate with strings.

Because they cannot handle Unicode in any meaningful way,
I have changed their implementation to not even attempt it,
as it is more complicated to handle Unicode, and it doesn't
even work anyway.
@dgpv

dgpv commented Oct 13, 2020

Copy link
Copy Markdown
Contributor

Just

def unhexlify_str(h):
   return binascii.unhexlify(h.encode('utf8'))

could be enough, IMO, as the code is very short and obvious.

But more documentation is not less, so I'm not complaining.

@ysangkok

Copy link
Copy Markdown
Contributor Author

@petertodd Hi, any possibility of having you look at this? I'd be ready to make any necessary changes.

@petertodd

Copy link
Copy Markdown
Owner

I haven't actually tested this. But looks fine and I'm going to unilaterally pull the plug on Py2. :)

@petertodd
petertodd merged commit 6980e17 into petertodd:master Nov 16, 2021
@ysangkok
ysangkok deleted the remove-py2-compat branch November 16, 2021 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants