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

Keep HTTPError as a base class for .request() and .raise_for_status() #1125

Merged
merged 1 commit into from
Aug 3, 2020

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Aug 3, 2020

Closes #1108, and following rationale in #1083 (comment), which I'll repeat here...

I think there's probably also a decent case to make for not deprecating it at all, and instead keeping it as a base class, but only for RequestError and HTTPStatusError, so that...

try:
    response = httpx.get("https://www.example.com/")  # May raise a RequestError subclass
    response.raise_for_status()  # May raise HTTPStatusError
except httpx.HTTPError as exc:
    pass

Which means existing code all keeps working as expected, while still providing finer-grained classes where needed.

It's also more concise than the alternative...

try:
    response = httpx.get("https://www.example.com/")  # May raise a RequestError subclass
    response.raise_for_status()  # May raise HTTPStatusError
except (httpx.RequestError, httpx.HTTPStatusError) as exc:
    pass

This hadn't really occurred to me when looking at #1095.

The typing is also still nice and clear here. Both HTTPStatusError and RequestError provide .request, so we can suitably annotate that on the base class. However accessing .response on httpx.HTTPError would be a typing error, since it could either be a RequestError (which isn't associated with a response) or a HTTPStatusError (which is).

The following are all valid...

response = httpx.get("https://www.example.com/")  # May raise a RequestError subclass
try:
    response.raise_for_status()  # May raise HTTPStatusError
except httpx.HTTPStatusError as exc:
    print(f"Unexpected status code in response: {exc.response.status_code}")
try:
    response = httpx.get("https://www.example.com/")  # May raise a RequestError subclass
    response.raise_for_status()  # May raise HTTPStatusError
except httpx.RequestError as exc:
    print(f"Request to {exc.request.url} failed: {exc.message}")
except httpx.HTTPStatusError as exc:
    print(f"Unexpected status code in response: {exc.response.status_code}")
try:
    response = httpx.get("https://www.example.com/")  # May raise a RequestError subclass
    response.raise_for_status()  # May raise HTTPStatusError
except httpx.HTTPError as exc:
    if isinstance(exc, HTTPStatusError):
        # The following line will type-check properly now.
        print(f"Unexpected status code in response: {exc.response.status_code}")
    else:
        print(f"Request to {exc.request.url} failed: {exc.message}")

I think this is probably all a usability win for our users given...

  • except httpx.HTTPError as exc: is more concise than except (httpx.RequestError, httpx.StatusCodeError) as exc:
  • It's a closer match for requests codebases that are migrating.
  • We're not breaking anything for our existing users here.

How do we feel about this?

@tomchristie tomchristie added the enhancement New feature or request label Aug 3, 2020
@tomchristie tomchristie added this to the v0.14 milestone Aug 3, 2020
@iwoloschin
Copy link

This looks great, with one question, is there a reason you're explicitly not using HTTPError as a base exception for all possible httpx exceptions?

@tomchristie
Copy link
Member Author

tomchristie commented Aug 3, 2020

is there a reason you're explicitly not using HTTPError as a base exception for all possible httpx exceptions?

Absolutely, yup. RequestError and HTTPStatusError are categories of exception that you may want to catch and handle.

The remaining cases are due to programmatic errors. It's not typically useful to catch and handle those cases, and including them in HTTPError means we'd be over-generalising a class that's really just intended for the particularly useful and common class of errors that are "could not make the request, or the response was 4xx/5xx".

Of the remaining cases...

  • NotRedirectResponse occurs if you're calling response.next() without properly checking .is_redirect_response first.
  • The StreamError cases occur if you're accessing request/response streams in some broken way, eg. trying to stream the body twice, trying to stream the body after having closed it etc.
  • CookieConflictError occurs if you need to be more specific in your response.cookies.get() access, by including domain=... and path=... arguments to narrow down which domain/path you want a cookie for. (Granted, this is a bit more ambiguous than the other two cases.)

@iwoloschin
Copy link

Ah, well, that explains why I've never really needed those other exceptions! I think that's a reasonable explanation and I'd recommend that something similar (or even just what you wrote) definitely be included in the documentation.

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.

Sounds about right to me! I'm also feeling we could keep this w/o deprecation, it's probably a nice handy feature (which I agree should be documented but I understand we haven't started documenting the exception hierarchy just yet).

@tomchristie tomchristie merged commit d7aa6e0 into master Aug 3, 2020
@tomchristie tomchristie deleted the httperror-base-class branch August 3, 2020 19:06
@michaeloliverx
Copy link
Member

The .request attribute was removed from HTTPError in #1522, was this intended?

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

Successfully merging this pull request may close these issues.

Should HTTPError be a base class for RequestError and HTTPStatusError?
4 participants