-
Notifications
You must be signed in to change notification settings - Fork 184
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
proxy: add support for scheme prefixes socks4:// and socks5:// in http_proxy environment variable #157
base: master
Are you sure you want to change the base?
Conversation
… http_proxy environment variable
Codecov Report
@@ Coverage Diff @@
## master #157 +/- ##
==========================================
+ Coverage 75.64% 75.74% +0.09%
==========================================
Files 8 8
Lines 2616 2626 +10
==========================================
+ Hits 1979 1989 +10
Misses 637 637
Continue to review full report at Codecov.
|
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.
Great idea, please add tests.
proxy_type = 3 # socks.PROXY_TYPE_HTTP | ||
if len(url.scheme) > 0: | ||
_scheme_prefix = url.scheme.lower() | ||
if _scheme_prefix in proxy_schemes: | ||
proxy_type = proxy_schemes[_scheme_prefix] |
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.
proxy_type = 3 # socks.PROXY_TYPE_HTTP | |
if len(url.scheme) > 0: | |
_scheme_prefix = url.scheme.lower() | |
if _scheme_prefix in proxy_schemes: | |
proxy_type = proxy_schemes[_scheme_prefix] | |
proxy_type = proxy_schemes.get(url.scheme.lower(), 3) # socks.PROXY_TYPE_HTTP |
@@ -74,6 +74,8 @@ | |||
ssl_SSLError = getattr(ssl, "SSLError", None) | |||
ssl_CertificateError = getattr(ssl, "CertificateError", None) | |||
|
|||
# socks.PROXY_TYPE_SOCKS4, socks.PROXY_TYPE_SOCKS5, socks.PROXY_TYPE_HTTP | |||
proxy_schemes = { 'socks4' : 1, 'socks5' : 2, 'http' : 3 } |
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.
Please use what black --diff python2/ python3/
says here
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.
Please put this after socks import or somewhere not between two ssl related blocks.
Thanks for taking the time to look at this so quickly and provide feedback. I agree with your suggestions but may not have time to make all these changes immediately. As the patch is quite small, would you consider accepting it without unit tests? I'm happy to move the definition of |
FYI, I tested this against the tor daemon running on the same host as the Python process. I set this in my environment:
and it works well. |
No worries, take your time. Tests are required not because I don't trust that it works, but because it should continue to work with all future changes. Luckily, there are good examples of checking proxy settings from environment, see here https://github.com/httplib2/httplib2/blob/master/tests/test_proxy.py |
No description provided.