- Notifications
You must be signed in to change notification settings - Fork673
refactor: move response_content into backend code#2488
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
codecov-commenter commentedFeb 12, 2023 • edited by codecovbot
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by codecovbot
Uh oh!
There was an error while loading.Please reload this page.
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 to940e59cCompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
940e59c to4dc7f00Compare0f907b9 toab089fbCompareab089fb toa8c95c8Compare@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!
This Pull Request (PR) was marked stale because it has been open 90 days with no activity. Please remove the stale label or comment on this PR. Otherwise, it will be closed in 15 days. |
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 🤔