-
Notifications
You must be signed in to change notification settings - Fork 644
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
refactor: move response_content into backend code #2488
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to keep the signature as before. Instead of using *args/**kwargs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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_content
completely from individual methods and just handle this in the request code 🤔