Should HTTPError be a base class for RequestError and HTTPStatusError?

I'm going through some of my projects preparing them for the 0.14 release and it occurred to me that the following line may be confusing and trip folks up.

httpx/httpx/_exceptions.py

Line 282 in 9409900

HTTPError = RequestError

This will break something like the following since the HTTPStatusError is now no longer caught, it'd just raise straight up and crash the program.

try:
    r = httpx.get('https://httpbin.org/status/400')
    r.raise_for_status()
except httpx.HTTPError as error:
    print(error)

Instead, can we change the line to:

HTTPError = (HTTPStatusError, RequestError)

That way at least something close to (or maybe exactly like?) the original functionality is maintained, at least until it is formally deprecated?

Would need to include all the top-level exceptions from #1095, so:

HTTPError = (HTTPStatusError, RequestError, NotRedirectResponse, CookieConflict, StreamError)

That's a really interesting idea, thanks!

An alternative might be to keep it as an exception class, as a base class of RequestError and HTTPStatusError?

An alternative might be to keep it as an exception class, as a base class of RequestError and HTTPStatusError?

I'd support this option.

Actually, yeah, an exception class is probably better, you could probably do something to print out a deprecation warning that way?

I guess theres a question here, re: if we should actually prefer to have that as a base class permanently, or if we'd only be doing this to capture both cases during a gentle deprecation.

Incidentally I'm not sure there is any way we'd be able to raise a deprecation warning to users importing and using an exception class in an except statement.

Incidentally I'm not sure there is any way we'd be able to raise a deprecation warning to users importing and using an exception class in an except statement.

Actually I think there's a way: Module __getattr__. :-) This use case of managing deprecation warnings on module attributes is actually the flagship behind the PEP, it seems. This feature is 3.7+ only, but can we afford a hard-break on 3.6 maybe…?

# httpx/__init__.py

def __getattr__(name: str):  # type: ignore  # Called if `httpx.<name>` was not found
    if name == "HTTPError":
        from ._utils import warn_deprecated

       # (Not sure about this error message, considering we might want to point at "it could be any of the top-level exceptions, pick one explicitly".)
        warn_deprecated("HTTPError is deprecated, please use one of RequestError or HTTPStatusError instead")
        from ._exceptions import HTTPError

        return HTTPError

    raise AttributeError(f"module {__name__} has no attribute {name}")

Result:

$ python -Wo -c "import httpx; print(httpx.HTTPError)"
/Users/florimond/Developer/python-projects/httpx/httpx/__init__.py:45: DeprecationWarning: HTTPError is deprecated
  warn_deprecated("HTTPError is deprecated")
<class 'httpx._exceptions.RequestError'>

if we should actually prefer to have that as a base class permanently, or if we'd only be doing this to capture both cases during a gentle deprecation.

I think we should only keep HTTPError as deprecation scaffolding. To me, the key idea of the work on the exception hierarchy was to more clearly separate between "errors that can occur when making a request" and "the developer made a mistake". If we keep HTTPError, users won't be nudged into making that separation clear in their code (they'd repeat what they've been used to do with Requests?), and we'd stay in the previous Requests-inherited state of "some developer mistakes may and will go un-noticed".

My only goal here was to highlight a relatively innocent change that would wind up being a horribly breaking change. To be fair, pre-1.0 breaking changes are completely fair game, but it's nice to at least alert users to the breaking changes!

That being said, I do like the concept of "one base exception per library", but in httpx's case I think that having a few base exceptions is reasonable. For instance, some of the code I was updating when I realized this might be a problem looks a lot better than the original code with only HTTPError. Also, I've just started using Pylance and it is much happier with the split exceptions since it is now completely unambiguous whether a .response is present or not.

Navigation Menu

Repository navigation

Should HTTPError be a base class for RequestError and HTTPStatusError? #1108

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Should HTTPError be a base class for RequestError and HTTPStatusError? #1108

Description

Activity

StephenBrown2 commented on Jul 31, 2020

lovelydinosaur commented on Jul 31, 2020

StephenBrown2 commented on Jul 31, 2020

iwoloschin commented on Jul 31, 2020

lovelydinosaur commented on Aug 1, 2020

florimondmanca commented on Aug 1, 2020

florimondmanca commented on Aug 1, 2020

iwoloschin commented on Aug 1, 2020

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions