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

On proxies={"http": ...} vs. proxies={"http://": ...} #1105

Closed
tomchristie opened this issue Jul 31, 2020 · 1 comment · Fixed by #1127
Closed

On proxies={"http": ...} vs. proxies={"http://": ...} #1105

tomchristie opened this issue Jul 31, 2020 · 1 comment · Fixed by #1127
Labels
user-experience Ensuring that users have a good experience using the library
Milestone

Comments

@tomchristie
Copy link
Member

tomchristie commented Jul 31, 2020

Currently we're in line with requests in supporting "all", "http", and "https" as valid proxy keys.

Internally we handle those as URL patterns that we can match against, and eg. "http" is actually mapped to "http://".

I'm wondering if we might have better internal consistency by...

  • Raising a warning on base usage of "all", "http", "https", later escalating this to an error.
  • Instead requiring an explicit "all://", "http://", "https://", all of which properly parse directly as URLs.

My feeling is that doing so would create a nicer consistency, and also has a subtle indication to developers about what is actually going on here.

For example...

proxies={"http://": httpx.Proxies(...)} neatly implies that proxies={"http://www.example.com": httpx.Proxies(...)} might also be a valid thing to do, which indeed it is.

Allowing proxies={"http": httpx.Proxies(...)} makes it less obvious that the key is a general purpose URL pattern.

Additionally this change means that other usages will end up with more expected behaviors, eg. proxies={"www.example.com": ...} will do what you'd expect, rather than mapping to proxies={"www.example.com://": ...} which is what we currently do. - Not true, that'd parse as scheme='', host='', path='www.example.com'. Instead we'd probably want to be strict and ensure that only absolute URLs are allowed as pattern keys. (While still including a graceful deprecation for plain "http", "https", "all")

This becomes doubly relevant once we introduce a mount API, which will use the same URL pattern style for it's API, as current proxies=... argument.

@tomchristie tomchristie added the user-experience Ensuring that users have a good experience using the library label Jul 31, 2020
@tomchristie tomchristie added this to the v0.14 milestone Jul 31, 2020
This was referenced Aug 2, 2020
@cdeler
Copy link
Member

cdeler commented Aug 4, 2020

Hello @tomchristie ,

Are you talking about the changes in httpx._client.BaseClient._get_proxy_map (here: link)
Also, what should we do with get_environment_proxies? (link) This function requires envs like HTTPS_PROXY etc.

UPD: @tomchristie I've created a PR just to show my idea. If I'm wrong, feel free to discard it

cdeler added a commit to cdeler/httpx that referenced this issue Aug 4, 2020
cdeler added a commit to cdeler/httpx that referenced this issue Aug 4, 2020
cdeler added a commit to cdeler/httpx that referenced this issue Aug 4, 2020
cdeler added a commit to cdeler/httpx that referenced this issue Aug 4, 2020
cdeler added a commit to cdeler/httpx that referenced this issue Aug 5, 2020
…use proxies={"http": ...} instead of proxies={"http://": ...}
cdeler added a commit to cdeler/httpx that referenced this issue Aug 5, 2020
…ies={"http": ...} instead of {"http://": ...}. Updated docs and added unit, which check the warning presence
cdeler added a commit to cdeler/httpx that referenced this issue Aug 5, 2020
…ies={"http": ...} instead of {"http://": ...}. Updated docs and added unit, which check the warning presence
tomchristie added a commit that referenced this issue Aug 5, 2020
* #1105 added deprecation warning, raised when we try to use proxies={"http": ...} instead of {"http://": ...}. Updated docs and added unit, which check the warning presence

* Update tests/client/test_proxies.py

Co-authored-by: Florimond Manca <[email protected]>

* Update tests/client/test_proxies.py

Co-authored-by: Tom Christie <[email protected]>
Co-authored-by: Florimond Manca <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user-experience Ensuring that users have a good experience using the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants