-
Notifications
You must be signed in to change notification settings - Fork 330
drop six #294
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
drop six #294
Conversation
This pull request introduces 1 alert when merging 2f36ee9 into 6c20190 - view on LGTM.com
new alerts:
- 1 for Syntax error
2f36ee9 to
f866bdd
Compare
Sorry, but we don't plan dropping python2 support any time soon.
@xiaoge1001 thanks for your patch, it has been included on Debian
Actualy, six is not needed for b("literal"), b"literal"also works on Python2;
these lines could already be now merged.
yes change to b'literal' is definitely something we can do (and you can see, happens in new code), but there are things like int_types that are still needed for py2 compatibility.
Yes yes... I'm almost done withj my pr "All those moments will be lost in time, like tears in rain."
Reducing the diff of 80%~90% would make life of downstreams easier.
@cjwatson
cjwatson
Apr 30, 2024
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaks the if GMPY2 or GMPY case: it tries to do integer_types = tuple(integer_types + (type(mpz(1)),)) and gets a NameError as a result. I suggest adding integer_types = int, somewhere up here and reverting the other removals of integer_types in this file, as I just did in https://salsa.debian.org/python-team/packages/python-ecdsa/-/commit/3a95d8623d28d73c8c7877e6c3ddd5ee43e33ca4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't be against putting integer_types into _compat.py instead of using it from six, feel free to open a new PR that does that
do it. #293