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

Httpcore interface #804

Merged
merged 11 commits into from
Apr 8, 2020
Merged

Httpcore interface #804

merged 11 commits into from
Apr 8, 2020

Conversation

tomchristie
Copy link
Member

Working towards switching the dispatcher interface to the proposed httpcore interface. https://www.encode.io/httpcore/

@tomchristie
Copy link
Member Author

Turns out this was a bit of a monster, so I've taken a different tack to get moving here.

Rather than adapting httpx, and then pulling into httpcore once the interfaces match up, I've instead started on the implementation in httpcore, pulling stuff across from here as needed...

encode/httpcore#11

At the moment there's a very basic sync + async HTTP/1.1 only implementation.

@tomchristie tomchristie mentioned this pull request Feb 27, 2020
3 tasks
@tomchristie tomchristie marked this pull request as ready for review March 5, 2020 16:23
@tomchristie
Copy link
Member Author

Alrighty, so this branch now has working integration against httpcore.

Not going to suggest that we merge it yet, but pretty happy that we're at this point.

Some things to consider...

  • We'll want to document the low-level interface alongside this, even if that's mostly just pointing at httpcore for that.
  • Should we be switching from dispatch=... to transport=...? (And eg. WSGITransport, ASGITransport, URLLib3Transport classes?)
  • We'll want to expose the urllib3 option as a custom transport class, documenting how to use that.
  • We'll need to document dropping UDS in the release notes, along with the motivation of why we're making that trade-off.
  • We may want to do a bit of going over the httpcore interface and making sure we're super happy with that all. In particular we currently return the reason phrase info in that. It's not totally clear if that's something that we do or don't want to do. (Won't go into that in depth here tho.)
  • We may want to go over the exception classes in httpcore, and make sure we think we've got those mapped out sensibly, and not missing anything.

We'll also almost certainly want to start off with a pre-release version of this, since it's a big amount of churn. Also I'd really like to see httpcore getting some better test coverage. It's got a base level of integration tests, but the proxy implementation flat-out isn't tested at the moment, which is pretty duff.

Anything else to think about at this stage?

@tomchristie tomchristie added the refactor Issues and PRs related to code refactoring label Mar 5, 2020
@tomchristie
Copy link
Member Author

We'll also want to drop the "The HTTPX client provides HTTP/2 support, which is currently only available with the async client." note in the docs once this goes in, since we've got HTTP/2 support in both cases in httpcore.

@victoraugustolls
Copy link

Do you think this branch is ready to make the tests #832 and #841 and see if it helped?

@tomchristie
Copy link
Member Author

Worth having a look at, sure. I’ve not dig into those issues do not a lot to say on them myself yet. Let’s keep any follow-up observations on those those threads, rather than here.

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Impressive amount of work. This is obviously a whole lot to review, not sure how much you're expecting to receive. :) 100% yes on the idea of a pre-release/public beta anyway.

tests/client/test_auth.py Outdated Show resolved Hide resolved
@tomchristie
Copy link
Member Author

Yeah, I’d love to be able to do this in incremental, sensibly reviewable stages, but I don’t think it falls into that easily.

@florimondmanca
Copy link
Member

I'd still like to go and review this more thoroughly, but it's hard to find the time.

@tomchristie Separate note — is the plan to release the .dev0 pre-release from this branch directly, and then call for beta testing?

@tomchristie
Copy link
Member Author

is the plan to release the .dev0 pre-release from this branch directly, and then call for beta testing?

I think that'd probably be a sensible approach, yup. Sound okay with you?

@florimondmanca
Copy link
Member

Sure!

@tomchristie
Copy link
Member Author

Soo.... I think it'd actually be a good plan for us to merge this to master before issuing a 0.13 pre-release. That'll ensure we're not getting out of sync as new stuff comes in.

Are we okay to do that right now, or?

Copy link
Member

@florimondmanca florimondmanca left a comment

Choose a reason for hiding this comment

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

Looks good, let's go! 🚀

@florimondmanca
Copy link
Member

florimondmanca commented Mar 29, 2020

@tomchristie Actually, should we release 0.12.2 first with pending fixes in master, and then issue a 0.13 pre-release? (There aren't any pressing fixes — see diff — so we can just as well batch everything up as 0.13.)

@tomchristie tomchristie merged commit 3046e92 into master Apr 8, 2020
@tomchristie tomchristie deleted the httpcore-interface branch April 8, 2020 12:32
@tomchristie tomchristie mentioned this pull request May 21, 2020
jcugat added a commit to jcugat/httpx that referenced this pull request May 27, 2020
jcugat added a commit to jcugat/httpx that referenced this pull request May 27, 2020
jcugat added a commit to jcugat/httpx that referenced this pull request May 30, 2020
florimondmanca pushed a commit that referenced this pull request May 30, 2020
* Replace remaining occurrences of dispatch with transport

* Remove unused AsyncDispatcher

Was removed in #804

* Remove hard_limit warning in test
florimondmanca added a commit that referenced this pull request May 30, 2020
* Remove unused/untested headers copy() method

Last usage was removed in #804

* Remove unused premature_close server endpoint

Last usage was removed in #804

* Increased test coverage

* Revert removal of headers copy() method

Documented and added tests for it.

Co-authored-by: Florimond Manca <florimond.manca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor Issues and PRs related to code refactoring
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants