fix(httpx): resolve conflict between default transport and proxy settings #969
Conversation
karpetrosyan
changed the title
|
|
||
| mounts = tuple(client._client._mounts.items()) | ||
| assert len(mounts) == 1 | ||
| assert mounts[0][0].pattern == "https://" |
There was a problem hiding this comment.
nit: can we include a check for the hostname here as well to make sure it's set to example.org?
There was a problem hiding this comment.
It's actually not a string, but a transport object that hides how it retrieves the response. I decided to avoid checking the hostname to prevent diving into deep internals that might change frequently and break compatibility. I'm only checking the ._mounts property, which I believe will remain stable.
src/anthropic/_base_client.py
Outdated
| default_transport = AsyncHTTPTransport(**transport_kwargs) | ||
|
|
||
| # Prioritize the mounts set by the user over the environment variables. | ||
| kwargs["mounts"] = proxy_mounts | kwargs.get("mounts", {}) |
There was a problem hiding this comment.
nit: I think the | syntax here is Python 3.9 only? We currently support 3.8
| proxy_map = {key: None if url is None else Proxy(url=url) for key, url in get_environment_proxies().items()} | ||
|
|
||
| transport_kwargs = { | ||
| arg: kwargs[arg] for arg in ("verify", "cert", "trust_env", "http1", "http2", "limits") if arg in kwargs |
There was a problem hiding this comment.
would it be feasible to add unit tests for all of these args as well?
There was a problem hiding this comment.
I guess it's the same problem here. The transport doesn't expose any attributes, so we could write some references to the internal variables if it's worth it. But I think we can avoid that because we are just testing here whether the arguments were correctly passed to httpx.
There was a problem hiding this comment.
There was a problem hiding this comment.
Added a simple smoke test to ensure there are no exceptions when providing arguments to the default client. Also suppressed the deprecation warning because httpx deprecated cert and will remove it in the major release.
RobertCraigie
left a comment
RobertCraigie
left a comment
There was a problem hiding this comment.
Very nice, thank you!
2 participants
closes #878