-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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 from environment variables #1825
Conversation
@@ -96,7 +98,7 @@ def __init__(self, method, url, *, | |||
self.update_cookies(cookies) | |||
self.update_content_encoding(data) | |||
self.update_auth(auth) | |||
self.update_proxy(proxy, proxy_auth) | |||
self.update_proxy(proxy, proxy_auth, proxy_from_env) |
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.
let's use False instead of None
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.
ok. I'll wait for more comments and push the changes
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've added the change and squashed a bit the commit history
Codecov Report
@@ Coverage Diff @@
## master #1825 +/- ##
==========================================
+ Coverage 97.23% 97.23% +<.01%
==========================================
Files 37 37
Lines 7478 7482 +4
Branches 1299 1300 +1
==========================================
+ Hits 7271 7275 +4
Misses 87 87
Partials 120 120
Continue to review full report at Codecov.
|
Update changes and contributors Escaping spell-checking Set default to False
LGFM, though I'm pretty sure people will get annoyed by I think that flag should be described quite notable since it's potential gotcha. |
@kxepal this is also fully my opinion and why I reported it as an issue in #1791 !!! But as pointed in the various discussions, @fafhrd91 and @asvetlov are strictly against falling back to environment variables... Unfortunately for now, aiohttp doesn't even allow this possibility, so this is a first step. This WAS indeed a real gotcha towards |
@MRigal Sorry, I didn't follow those discussions. But indeed, that's a good step forward. |
good work! |
What do these changes do?
Allows optional reading proxy from environment variables. Addresses #1791 #529 and #102
Are there changes in behavior for the user?
They can use proxy from environment variables like in requests, even if it is disabled by default here
Related issue number
Resolving open issue #1791
Checklist
CONTRIBUTORS.txt
CHANGES.rst
#issue_number
format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.