-
Notifications
You must be signed in to change notification settings - Fork 28
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
return response instead of raw for stream requests #245
Conversation
b17d91d
to
2062d34
Compare
- enables use of iterator - https://requests.readthedocs.io/en/latest/user/quickstart.html\#raw-response-content Signed-off-by: Alex Kwiatkowski <alex+git@fremantle.io>
2062d34
to
a1770cb
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #245 +/- ##
=======================================
Coverage 83.79% 83.79%
=======================================
Files 34 34
Lines 2117 2117
=======================================
Hits 1774 1774
Misses 343 343
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
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.
@rupurt Thanks for your contribution!
Although this is a breaking change for anyone using SDK methods that return streams, it seems to follow the best practices recommended by the requests
package 👍
@t1m0thyj yeah, that would be unfortunate. But I feel like this is something worth making a breaking change for. Currently you need to manually decode the gzipped responses. If we want to avoid a breaking change, how do you feel about an extra argument |
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 agree with the position of just making the breaking change.
We do want to handle as much as it makes sense, and also follow recommended practices.
LGTM! 😋
P.S. I don't think we need an extra parameter 😋
What It Does
How to Test
Review Checklist
I certify that I have: