Skip to content

Conversation

@tbascoul
Copy link
Contributor

Make the response's request parameter optional

Part of #1227

@lovelydinosaur
Copy link
Contributor

Okay, so it turns out this issue is actually more awkawrd than realised at first, because although we do want request=... top be optional on a Response() instance, we don't want it to be optional on RequestError/HTTPError exceptions, where we know it will always be set.

Here's what I think we're going to need to do here...

  • The signatures on our exception classes should not change.
  • The decoders should not raise DecodeError, or accept a request instance on __init__. Instead any decoding errors should just raise a plain old ValueError exception.
  • The Response instance should wrap any decoding codeblocks instead a try ... except ValueError as exc. If response._request is set then the exception should be re-raised using DecodeError(message=str(exc), request=self.request) from exc, but if response._request is not set, then the plain ValueError exception should just be re-raised, using raise exc.

That'll allow us to have...

  • An optional request=... parameter on the Response.__init__().
  • Without changing the exception signatures.
  • Properly raising httpx.DecodeError() on exceptions that occur during a request flow, where we will have an associated request instance.
  • For unit-test style response instances, that do not have a request=... instance set, we'll get plain ValueError exceptions for any decoding errors.

You're welcome to take on these follow ups yourself, or else I'd also be happy to take a look at them.

@tbascoul
Copy link
Contributor Author

Thanks for the detailed guidance.
Still some work to do on the testing part.

httpx/_models.py Outdated
Comment on lines 861 to 863
if self._request is None:
raise ValueError(message)
raise HTTPStatusError(message, request=self._request, response=self)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raising a ValueError inside raise_for_status seems a bit off ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest we start the method with something like...

if self._request is None:
    raise RuntimeError("Cannot call `raise_for_status` as the request instance has not been set on this response.")

Copy link
Contributor

@lovelydinosaur lovelydinosaur left a comment

Choose a reason for hiding this comment

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

Great, yup! Thanks so much for your time on this!

I've added in a _wrap_decode_errors context manager, to remove some of the replication, and get the test coverage back up to 100%

@lovelydinosaur lovelydinosaur merged commit e0b4528 into encode:master Sep 1, 2020
@lovelydinosaur lovelydinosaur mentioned this pull request Sep 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants