-
-
Notifications
You must be signed in to change notification settings - Fork 862
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
Consider URLLib3Transport
interface, since it's becoming public API.
#966
Comments
Also we could just drop the And in any case, never made it to the public interface even then. |
The only reason I'd be in favor of keeping I'm actually not certain that's what people would do in practice - there are already lots of behavior differences that come from switching from Requests anyway, so not sure if switching to our transport is a big leap. Another argument in favor of dropping it before 1.0 lands would be reducing maintenance burden - now that the transport interface is public it would be pretty easy for someone to maintain this as a 3rd party component (esp given that we have a working implementation in 0.12.x for people to take inspiration from). But at the same time, it's not very high cost. Edit: oh okay, I didn't get that you were talking about the |
I'm 👍 on dropping I might be missing something but it seems atm it feels like the remnants of our old sync flow rather than a core piece of HTTPX that users would reach for. It already has our lowest test coverage percentage, feels like it will eventually break for someone since it's now in the public API. |
Okay, well let's drop the (Personally I probably think it's a pretty useful thing to have available to us, but I'm not absolute on it.) |
Remaining points for 1.0 consideration...
|
We can close this off now. Once #1090 is addressed, the signatures are... class URLLib3Transport(httpcore.SyncHTTPTransport):
def __init__(
self,
*,
verify: VerifyTypes = True,
cert: CertTypes = None,
trust_env: bool = None,
pool_connections: int = 10,
pool_maxsize: int = 10,
pool_block: bool = False,
): And... class URLLib3ProxyTransport(URLLib3Transport):
def __init__(
self,
*,
proxy_url: str,
proxy_headers: dict = None,
verify: VerifyTypes = True,
cert: CertTypes = None,
trust_env: bool = None,
pool_connections: int = 10,
pool_maxsize: int = 10,
pool_block: bool = False,
): |
Follow on from #963
There's a couple of things we could consider in the
URLLib3Transport
interface, now that it's becoming public API.pool_limits
, since it doesn't quite fit the parameters thaturllib3
uses to configure it's PoolManager. We needed to pass it before, since we were using the class for our sync implementation, and needed the consistency between our async and sync cases, but now that we're providing it separately we should just mirror their nativenum_pools
andmaxsize
.httpcore
we provide different classes for the proxy and the plain-connection-pool transports. We could? choose to do the same with theurllib3
transport, rather than providing a single class with an optionalproxy=...
argument.The text was updated successfully, but these errors were encountered: