refactor: move response_content into backend code #2488
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2488 +/- ##
==========================================
- Coverage 96.52% 96.10% -0.43%
==========================================
Files 90 87 -3
Lines 5872 5667 -205
==========================================
- Hits 5668 5446 -222
- Misses 204 221 +17
Flags with carried forward coverage won't be shown. Click here to find out more.
|
70a5cfd to
940e59c
Compare
940e59c to
4dc7f00
Compare
0f907b9 to
ab089fb
Compare
ab089fb to
a8c95c8
Compare
|
@JohnVillalovos I've rebased and reworked this a bit based on previous comments. Would you be able to have a look at this one? IMO it's also a bit cleaner like this to have the response logic be part of the client/backend than a separate util function since we've split out the client code (and more recently the backend code) from the monolith |
There was a problem hiding this comment.
Thanks @nejch
Overall looks good to me. Only a couple of nits on my end. But I would be okay with merging as is. So approved by me.
| @@ -17,6 +17,17 @@ def __init__(self, response: requests.Response) -> None: ... | |||
|
|
|||
|
|
|||
| class Backend(Protocol): | |||
| @staticmethod | |||
| @abc.abstractmethod | |||
| def response_content( | |||
There was a problem hiding this comment.
This seems like it might be a good time to make the method require keyword args for all the args. I'm a fan of requiring keyword args 😆
There was a problem hiding this comment.
I believe this would be a breaking change as we still expose it in utils as a non-private function. In that case I'd maybe drop the wrapper entirely and not just deprecate it, as then we don't need to worry about maintaining the signature twice (#2488 (comment)).
Might be worth grouping breaking changes and wait to merge them all together though for a major release.
There was a problem hiding this comment.
Well I didn't think it would be a breaking change if this is the new function and then the wrapper (with the deprecation) would call this one with keyword args.
| chunk_size: int, | ||
| *, | ||
| iterator: bool, | ||
| *args: Any, **kwargs: Any |
There was a problem hiding this comment.
I would prefer to keep the signature as before. Instead of using *args/**kwargs.
There was a problem hiding this comment.
My initial idea was to just leave a simple wrapper for backward compatibility but maybe we can just drop it (see #2488 (comment)).
There was a problem hiding this comment.
My thought was that this one would maintain the same exact signature as before, and then call the new function with keyword arguments.
But! It isn't that important and perfectly fine with me if you want to merge this in as is 👍
There was a problem hiding this comment.
Ahh I misunderstood your comment I think, I thought you wanted to match the signatures. I'll add the changes!
Needed in order to later add other backends that handle streaming responses differently. Just a first step, later maybe we could get rid of
response_contentcompletely from individual methods and just handle this in the request code 🤔