Skip to content
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

Python 3 compatibility #7

Merged
merged 4 commits into from Nov 14, 2019
Merged

Python 3 compatibility #7

merged 4 commits into from Nov 14, 2019

Conversation

frenzymadness
Copy link
Contributor

This new pull request is based on the initial effort to make this library Python 3 compatible in PR #6 .

Tests work with a local instance of Danted Socks5 proxy with both Python 2.7 and 3.

Anyone who can test it or provide settings for more testing scenarios, please do.

SunilMohanAdapa and others added 4 commits September 19, 2019 21:11
- Use print as function by importing from future.

- Use six wrapper library to support urllib functions on Python 2 and Python 3.

Signed-off-by: Sunil Mohan Adapa <sunil@medhas.org>
socket._fileobject does not exist in Python 3 and makefile
method in stdlib uses SocketIO as well:
https://github.com/python/cpython/blob/3.7/Lib/socket.py#L221
@BjarniRunar
Copy link
Collaborator

Cool! Thanks for doing this. I will test it (and probably merge) as soon as I can.

@lzap
Copy link

lzap commented Oct 10, 2019

Thanks guys for doing this. When you merge this @BjarniRunar can I ask for a quick release (or some pre-release at least) so I can bump package in Fedora to be Python3 only?

@frenzymadness
Copy link
Contributor Author

Can we please merge this PR and issue a new release?

@BjarniRunar
Copy link
Collaborator

To be honest, no, this isn't just getting merged until I have time to focus on it.

I have to clean it up because you didn't actually care about the backwards compatibility and just threw something together that works for you. Maybe dropping backwards compatibility is the right thing to do, but then leaving all that legacy python 2.x (for x << 7) code in there is really sloppy.

Either way, doing this properly is work and I have to find time to do it.

@frenzymadness
Copy link
Contributor Author

I completely understand your approach but I cannot do more to test it because I am not a regular user of this tool. Moreover, I tried to keep backward compatibility as much as I can (that's the reason for six module here). Could you please tell me where I'm breaking compatibility with Python 2.7 and/or provide more tests so I can enhance my patch?

@pagekite pagekite merged commit 0e19e3a into pagekite:master Nov 14, 2019
@BjarniRunar
Copy link
Collaborator

I've merged this. Thanks everyone for the contributions!

My pushback wasn't that there was lots of work necessary, it was that I needed time to focus so I actually understood what is going on. Reading it I immediately saw some issues and when that's the case it's natural to assume there are more. There were, but they were all minor, so that's great.

Sorry I didn't answer your question, but figuring out the answer was the exact same amount of work as just fixing it.

@frenzymadness
Copy link
Contributor Author

Thank you!

@BjarniRunar
Copy link
Collaborator

Just a heads' up to @lzap and other interested parties - I do not consider this work quite finished yet. Please track #8 if you want to be notified when I think this is actually all working!

There will also be a new release tagged, 2.1.0.

@lzap
Copy link

lzap commented Nov 20, 2019

Thank you very much for the heads up - keep me informed and once everything is released I will put the package back immediately.

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.

None yet

5 participants