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

Make the response's request parameter optional #1238

Merged
merged 9 commits into from
Sep 1, 2020

Conversation

tbascoul
Copy link
Member

Make the response's request parameter optional

Part of #1227

@tomchristie
Copy link
Member

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
Member 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
Member 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
Member

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.")

httpx/_models.py Outdated Show resolved Hide resolved
Copy link
Member

@tomchristie tomchristie 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%

@tomchristie tomchristie merged commit e0b4528 into encode:master Sep 1, 2020
@tomchristie tomchristie 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