Repository navigation
Some API oddities/possible oversights #365
Description
While I was working on porting my google-music library to httpx using pared down oauth2 code borrowed from requests-oauthlib (see the quick stab at it here if interested), I noticed some API oddities/differences from requests even with the relatively small amount of usage in my library.
-
BasicAuthMiddleware, previouslyHTTPBasicAuth, is not available at the top level of the package, nor any other middleware aside fromDigestAuth. The current naming inconsistency between the two is also strange. Both are middleware, and it was namedHTTPBasicAuthin requests and previously in httpx. Though I could certainly see the inconsistency beingDigestAuthas all other middleware, includingCustomAuthMiddleware, follow the same pattern. I think something needs to happen as far as the exposing, organizing, and making consistent of things like this. What that thing is, I'm not sure. Quick opinion is that these classes being middleware is more of an implementation detail than anything else. I'd expect the auth classes to be exposed throughauthlike previously and in requests or at the top level. I'd also expect them without "Middleware" in the names (possibly just aliased). -
HTTPErroris not available at the top level like the other exceptions and like it was in requests. This is a somewhat common exception to catchraise_for_status, so having to add another import statement for it is a bit odd if intentional. -
requests.Sessionhas aparamsattribute that attaches those params to each session request. This is not present in httpxClientclasses like with headers/cookies. This is a useful feature for this and other use cases of mine and a divergence from requests.
Thanks for all the work. Been excited about httpx since the beginning. Glad to finally start using it more. Interested in seeing the discussion around these points.
Activity
florimondmanca commentedon Sep 21, 2019
Thanks @thebigmunch, IMO these are all valid points. :)
florimondmanca commentedon Sep 21, 2019
I opened separate issues for the last two points you reported.
As for the auth issues:
I do agree that we should be hiding the "middleware" part of it, as it is indeed an implementation detail, because the middleware API is still internal.
As far as naming is concerned, the point in time we decided to use
DigestAuthinstead ofHTTPDigestAuthis here: Digest auth middleware #305 (comment).The rationale was mostly that
DigestAuthlooks better. I personally do agree that theHTTPprefix seems arbitrary, but that also means we do break Requests compatibility there.So, two options:
HTTPDigestAuth,HTTPDigestAuth, andHTTPCustomAuth. (The latter did not exist in Requests, as auth customization went through theAuthBaseclass.)BasicAuth,DigestAuthandCustomAuth, but expose aliases forHTTPBasicAuthandHTTPDigestAuth. This kind of breaks the principle of having one obvious way to do things, so I think we'd have to raise some kind of warning on the aliased classes, encouraging users to rename?I'd be fine with re-exporting the auth classes under a public
httpx.authmodule, to comply with Requests and also make it more obvious where those classes should be imported from. (Again, importing them fromhttpx.middlewareas we do currently exposes an implementation detail.)Happy to read thoughts from other @encode/httpx-maintainers on this. :)
thebigmunch commentedon Sep 21, 2019
I also agree that the
HTTPis unnecessary as well, for what it's worth. This is an HTTP client package namedhttpxafter all : PI also don't believe a strict 1:1 compat with requests is necessary or necessarily preferable. Frankly, for all the good parts of its API, there is also plenty that could have been better (e.g. see Brett Cannon's blog post on http request API's). Instead, take a look, as we are now, at user experience and expectations. Using an
authmodule, even with a different class name, will be the obvious place a user would look for auth-related functionality.If I were going to do it, I'd move the
BaseMiddlewareclass to the top-levelmodelsmodule as that's really what it is here. Then move the auth classes to anauthmodule. Expose/moveRedirectmiddleware wherever you see it fitting best or let it stay where it is. I have less of an idea of what middleware might come in the future.sethmlarson commentedon Sep 21, 2019
Hey @thebigmunch! I'll start off by saying thank you for being one of the first to take the Requests -> HTTPX migration path, especially for a library. Your perspective is invaluable ❤️.
I'm kinda -1 about aliasing, we definitely need better docs about migrating from Requests though! About reexporting in the
authmodule, if it'll help users migrate then it sounds like a positive improvement.StephenBrown2 commentedon Sep 23, 2019
Aliasing but raising a
DeprecationErrorfor the aliases with a link to the migration docs would be a good middle path to take IMHO.subfxnet commentedon Nov 22, 2019
@florimondmanca Are there plans to make the client middleware API public?
I am using httpx as our API client library and would use the middleware concept to implement tracing into our client if it was a public interface.
florimondmanca commentedon Nov 22, 2019
@subfxnet There aren’t current plans to make it public, no, as we might be heavily reworking this part of HTTPX soon. I believe this is something we’d like to eventually provide, though.
lovelydinosaur commentedon Dec 5, 2019
Alrighty, I think we 're good to close this one off now...
httpx.BasicAuth(...)andhttpx.DigestAuth(...)available as top-level API now.httpx.HTTPExceptionavailable as top-level API now..paramsinterface availabe on the client.Thanks for helping round out some edges there!