Skip to content

Conversation

@ysangkok
Copy link
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.
@dgpv
Copy link
Contributor

dgpv commented Oct 13, 2020

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
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
Copy link
Contributor

dgpv commented Oct 13, 2020

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
Contributor Author

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

@petertodd
Copy link
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