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

Exception hierarchy #1095

Merged
merged 9 commits into from
Jul 31, 2020
Merged

Exception hierarchy #1095

merged 9 commits into from
Jul 31, 2020

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Jul 27, 2020

Refs #949

  • Decommissions the too-broad to be useful for anything HTTPError, in favour of an actually meaningful RequestError.
  • Rearranges the hierarchy as follows, dropping the RedirectError which encompassed two totally different kinds of error (one a coding error, the other a request error), and introducing TransportError.
  • Ensures consistently enforced attributes / __init__ parameters on all exceptions.

  • RequestError Exposes .request
    • TransportError
      • TimeoutException
        · ConnectTimeout
        · ReadTimeout
        · WriteTimeout
        · PoolTimeout
      • NetworkError
        · ConnectError
        · ReadError
        · WriteError
        · CloseError
      • ProxyError
      • ProtocolError
    • DecodingError
    • TooManyRedirects
    • RequestBodyUnavailable
    • InvalidURL
  • HTTPStatusError Exposes .request, .response
  • NotRedirectResponse
  • CookieConflict
  • StreamError
    • StreamConsumed
    • ResponseNotRead
    • RequestNotRead
    • ResponseClosed

@tomchristie tomchristie added the user-experience Ensuring that users have a good experience using the library label Jul 27, 2020
@tomchristie tomchristie added this to the v0.14 milestone Jul 27, 2020
@tomchristie tomchristie mentioned this pull request Jul 27, 2020
11 tasks
@tomchristie tomchristie requested a review from a team July 30, 2020 12:39
@florimondmanca florimondmanca self-requested a review July 30, 2020 16:37
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.

Wonderful ✨

The split between "request errors" vs "developer errors" is a real improvement over other HTTP clients (eg Requests crams everything into RequestError, so genuine developer errors typically unnoticed due to catch-all except requests.RequestError).

@@ -44,7 +50,8 @@ class DeflateDecoder(Decoder):
See: https://stackoverflow.com/questions/1838699
"""

def __init__(self) -> None:
def __init__(self, request: "Request") -> None:
self.request = request
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

? (Applicable to all other decoders here)

Suggested change
self.request = request
super().__init__(request)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah a guess a lot of other Python developers would tend always just rely on "hey I'm supposed to call super right". Personally, I think explicit is preferable to indirect here. Calling into super isn't always necessary or inherently the "right" thing to do.

Copy link
Member

@florimondmanca florimondmanca Jul 31, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, well then I guess Decoder would really be better served by a Protocol, rather than a full-fledged base class… But we only have them on Py38+, which is unfortunate.

class Decoder(Protocol):
    request: Request

    def decode(self, data: bytes) -> bytes:
        ...

    def flush(self) -> bytes:
        ...

But maybe we could then also use the request: Request annotation on the Decoder interface, remove its constructor, and add an explicit constructor for IdentityDecoder (for consistency with other decoder classes)?

Copy link
Member

@cdeler cdeler Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MultiDecoder inherits the Decoder interface, but do not have this property

From my point of view, MultiDecoder, TextDecoder and LineDecoder should be renamed, since they:

  1. on one hand these Decoders imitates the real decoders (having decode and flush)
  2. on the other hand these decoders violates Liskov's substitution principle (I don't want to be so nerd, mypy pointed it for me), e.g. as they accept different types as args of decode

httpx/_exceptions.py Outdated Show resolved Hide resolved
httpx/_exceptions.py Outdated Show resolved Hide resolved
httpx/_exceptions.py Outdated Show resolved Hide resolved
httpx/_exceptions.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user-experience Ensuring that users have a good experience using the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants