Improve error when redirecting with custom schemes#1002
Conversation
| url = URL(location, allow_relative=True) | ||
|
|
||
| # Check that we can handle the scheme | ||
| if url.scheme and url.scheme not in ("http", "https"): |
There was a problem hiding this comment.
This pattern ("http", "https") (or something similar) is repeated in a few places, like:
https://github.com/jcugat/httpx/blob/9160631d7fb957a319ce8985c97a858f8eab49a3/httpx/_client.py#L609
https://github.com/jcugat/httpx/blob/9160631d7fb957a319ce8985c97a858f8eab49a3/httpx/_models.py#L105
https://github.com/jcugat/httpx/blob/9160631d7fb957a319ce8985c97a858f8eab49a3/httpx/_models.py#L132
It would probably make sense to unify it in a single place.
There was a problem hiding this comment.
In these kinds of cases, if it's a single-liner I'd often prefer not to introduce indirection. It's as clear as it can possibly be, and it's not a complicated expression.
9160631 to
c57f992
Compare
yeraydiazdiaz
left a comment
yeraydiazdiaz
left a comment
There was a problem hiding this comment.
LGTM, thanks @jcugat!
4 participants
Fixes #822
I used
Scheme "blah" not supportedas the error message instead ofURL scheme must be "http" or "https".used in other parts of the code. It makes more sense to show the actual invalid scheme since it's coming from the server and not something that the user specified.I also tried the same test with
allow_redirects=Falsebut unfortunately when the response object is built, it needs a port which we can't derive from the custom scheme. Not sure how to solve this without changing the port definition ofUrlto allow aNoneport, ideas?