-
-
Notifications
You must be signed in to change notification settings - Fork 857
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
Drop URL(allow_relative=bool)
#1073
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left a few nit comments.
def test_get_invalid_url(server): | ||
with httpx.Client() as client: | ||
with pytest.raises(httpx.InvalidURL): | ||
client.get("invalid://example.org") | ||
with pytest.raises(httpx.InvalidURL): | ||
client.get("://example.org") | ||
with pytest.raises(httpx.InvalidURL): | ||
client.get("http://") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Nit) Considering using a parametrized test style? I enjoy the documentation purpose of id=...
, and that each case runs as its own test...
Then should we back-port this to test_async_client.py
as well?
def test_get_invalid_url(server): | |
with httpx.Client() as client: | |
with pytest.raises(httpx.InvalidURL): | |
client.get("invalid://example.org") | |
with pytest.raises(httpx.InvalidURL): | |
client.get("://example.org") | |
with pytest.raises(httpx.InvalidURL): | |
client.get("http://") | |
@pytest.mark.parametrize( | |
"url", | |
[ | |
pytest.param("invalid://example.org", id="scheme-not-http(s)"), | |
pytest.param("://example.org", id="no-scheme"), | |
pytest.param("http://", id="no-host"), | |
], | |
) | |
def test_get_invalid_url(server, url): | |
with httpx.Client() as client: | |
with pytest.raises(httpx.InvalidURL): | |
client.get(url) |
Co-authored-by: Florimond Manca <[email protected]>
This PR pushes all
url.scheme
andurl.host
checking down into the._transport_for_url
method.This allows us to drop the
allow_relative
flag on theURL
class, and instead just always allow relative URLs, up until the point that we're looking up a transport instance, at which point we enforce HTTP/HTTPS.There are two benefits here...
allow_relative=...
wasn't really ever intended as a bit of public API, we just happened to be relying on it ourselves. It's cleaner for our API for allow eghttpx.URL("")
as a valid relative URL, without having to explicitly enable it. But we do still want to raise an error if you attempt to issue a request with it.http
/https
schemes only, once we support mounting other scheme types too. Dropping the enforcement logic into the transport lookup means we'll be able to instead error if there's no appropriately mounted transport.