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

Add the 'proxy' parameter and deprecate 'proxies'. #2879

Merged
merged 23 commits into from
Dec 11, 2023

Conversation

karpetrosyan
Copy link
Member

@karpetrosyan karpetrosyan commented Oct 6, 2023

Summary

HTTPX supports proxies and mounts arguments, which are not the same, but mounts can simply replace proxies for complex routing, which is not always used.

So for simplicity, this PR deprecates proxies and adds proxy argument.
You can still use proxies, but it will warn you that it is deprecated and that you should use the new API.

Use...

  • proxy if you want to use a proxy server for all your requests.
  • mounts if you want to use a proxy server for requests matching the URL pattern you specified.

Proxy

import httpx

# instead of 
# httpx.Client(proxies="http://127.0.0.1:80")
# and
# httpx.Client(proxies={"all://": "http://127.0.0.1:80"})
# do ...
client = httpx.Client(proxy="http://127.0.0.1:80")

Mounts

import httpx

# instead of
# client = httpx.Client(
#    proxies={"http://": "http://127.0.0.1:80", "https://": "https://127.0.0.1:443"}
# )
# do ...
client = httpx.Client(
    mounts={
        "http://": httpx.HTTPTransport(proxy="http://127.0.0.1:80"),
        "https://": httpx.HTTPTransport(proxy="https://127.0.0.1:443"),
    }
)

@karpetrosyan karpetrosyan added the api change PRs that contain breaking public API changes label Oct 6, 2023
@karpetrosyan
Copy link
Member Author

@encode/maintainers If we want to switch to this API, I can work on the documentation.

I don't want to rush API changes, but if we believe the proxies API is not persistent, we should probably change it before 1.0.0, for example, deprecate it in the next release and remove it in 1.0.0.

@tomchristie
Copy link
Member

Yep seems like a clear API rationalisation to me, I'd be happy for us to make this switch for 1.0.

It woud probably also make sense for us to widen the available types for the proxy argument on httpx.HTTPTransport, so that the mount style is kept fairly simple...

client = httpx.Client(
    mounts={
        "http://": httpx.HTTPTransport(proxy="http://127.0.0.1:80"),
        "https://": httpx.HTTPTransport(proxy="https://127.0.0.1:443"),
    }
)

httpx/_transports/default.py Outdated Show resolved Hide resolved
httpx/_transports/default.py Show resolved Hide resolved
tomchristie and others added 2 commits October 11, 2023 09:35
Co-authored-by: T-256 <132141463+T-256@users.noreply.github.com>
Co-authored-by: T-256 <132141463+T-256@users.noreply.github.com>
@karpetrosyan
Copy link
Member Author

karpetrosyan commented Oct 11, 2023

Okay, so I can say that we should carefully consider how we will document this.
At this time, all of the routing staff described in the HTTP Proxies section do not make sense with the current implementation.
I propose that we change the documentation in a separate PR, and that we also document this feature there.

That new PR should answer this question:

  • What structure do we want for the HTTPX 1.0 documentation?
  • How do we want to document the deprecated things?
  • Do we want to describe custom transports more detailed? & Move the routing section into it.

I also want that PR will make HTTPX documentation more user-friendly and add some Mkdocs material features that can possibly make the documentation more readable and pleasant.

@tomchristie
Copy link
Member

Excellent questions, though quite wide ranging.

For the purposes of this pull request I'd suggest that...

  • The proxy routing docs move into the "mounting transports" section.
  • Deprecations are mentioned in the CHANGELOG.

@karpetrosyan karpetrosyan marked this pull request as ready for review October 12, 2023 07:29
httpx/_types.py Outdated Show resolved Hide resolved
Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good.

The command-line client also currently uses --proxies - I'd suggest we change that also.

For example here...

"--proxies [cyan]URL",

as well as in a few other places in that module.

@karpetrosyan
Copy link
Member Author

Do we want to keep the proxies argument in the cli app?
The migration is so easy; the user should just change the proxies word to proxy, so we can entirely remove it.

@tomchristie
Copy link
Member

the user should just change the proxies word to proxy

I'm happy with making that change, yes.

@tomchristie
Copy link
Member

Seems like a nice change to me, helps simplify things a bit.

Once we've merged this one we're prob committing ourselves to a major version we okay with that?

@karpetrosyan
Copy link
Member Author

Once we've merged this one we're prob committing ourselves to a major version we okay with that?

Let's put this in the next minor version for now, and then we'll completely switch to the new API with HTTPX 1.0 🎉

@karpetrosyan karpetrosyan mentioned this pull request Nov 23, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
karpetrosyan and others added 4 commits December 11, 2023 16:11
Co-authored-by: Tom Christie <tom@tomchristie.com>
Co-authored-by: Tom Christie <tom@tomchristie.com>
Co-authored-by: Tom Christie <tom@tomchristie.com>
@tomchristie
Copy link
Member

Looking great. Let's...

  • Update the description to indicate that proxies=... will continue to work at this point in time, but will issue a warning. (That's correct yeah?)
  • Should we drop proxies from the docstrings at this point, given that it's deprecated. Perhaps code comments alongside proxies=... indicating "# Deprecated. Remains functional, but will raise a warning if used."`?

@karpetrosyan
Copy link
Member Author

That's correct yeah?

Yes

Should we drop proxies from the docstrings at this point, given that it's deprecated. Perhaps code comments alongside proxies=... indicating "# Deprecated. Remains functional, but will raise a warning if used."`?

Is there a reason we need to get rid of docstring? I thought we were going to remove everything related to proxies with the major release.
We can look at how other encode projects deprecate things, but I don't think it's necessary.

@karpetrosyan karpetrosyan merged commit f8981f3 into encode:master Dec 11, 2023
5 checks passed
@karpetrosyan karpetrosyan mentioned this pull request Dec 18, 2023
@tomchristie tomchristie mentioned this pull request Jan 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api change PRs that contain breaking public API changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants