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

Tweak scheme handling in client #931

Closed
wants to merge 1 commit into from
Closed

Conversation

bz2
Copy link

@bz2 bz2 commented May 5, 2020

Remove hardcoded expectation that a scheme must be either 'http' or 'https' in BaseClient and URL classes. This moves towards being able to write dispatch handlers that have specific handling for differently named http-like schemes.

New BaseClient.accepted_schemes property that gives the collection of schemes a client can handle.

Test coverage for InvalidURL exception on unsupported schemes.

Motivation for the change comes from some test porting to httpx - where I found two cases where the current expectations made adapting existing code difficult:

  • Use of a test: scheme in unit tests for session handling.
  • Use of http+thing: custom schemes that enable additional behaviours at the transport level.

It may be doing this above the level of httpx client is wiser, but this branch as proposed seems reasonably harmless. Whether the (currently private) registry added for URL port mappings should be exposed for additions can be a question for later.

Remove hardcoded expectation that a scheme must be either 'http' or
'https' in `BaseClient` and `URL` classes. This moves towards being
able to write dispatch handlers that have specific handling for
differently named http-like schemes.

New `BaseClient.accepted_schemes` property that gives the collection
of schemes a client can handle.

Test coverage for InvalidURL exception on unsupported schemes.
@bz2
Copy link
Author

bz2 commented May 6, 2020

@tomchristie I can't set reviewers for this PR, but perhaps you could take a look? Thanks.

@tomchristie
Copy link
Member

Okay, this is interesting, but it's an area that I think we need some pretty careful thinking around...

The current PR only allows extra schemes by monkey patching a private variable, with @mock.patch.dict("httpx._models._port_registry", {"http+mock": 80}) which isn't really what we want.

Perhaps we could support an API such as Client(schemes={'http': 80, 'https': 443}) and allow users to alter that if they have a transport that supports some other scheme types. The schemes (or "supported_schemes" or "port_map" or something) could be passed to the URL instance too, and would be used to determine the .port attribute (which is used when calling into the transport API.)

@bz2
Copy link
Author

bz2 commented May 11, 2020

@tomchristie I agree it requires some thought to design an interface that makes sense.

That's why I think this private variable only change makes sense as a step forwards - collects together the related information, but does not tie you to supporting any mechanism to add new schemes, only a single read-only property.

It may be basically okay for the URL port mapping to be essentially global in the end? Would just want a way through Client of extending it before use.

@tomchristie
Copy link
Member

We need a bit more in depth discussion here - closing this off in favour of #954

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants