Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Support requests.response.raw being a file-like object #1094
Support requests.response.raw being a file-like object #1094
Changes from 13 commits
19e904b
f6bd84e
8600912
a7ac5b3
fafb6f0
3f8b212
6fcbcc8
e57adec
906ad39
b47412f
1ffc2aa
a5428ff
57997da
7635d52
93aa358
4e2e969
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
These should now be a
list
s.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.
Cookies were stored as a list in
response.raw._original_response.msg._headers
however with this change cookies are extracted fromresponse.headers.get('Set-Cookie', '')
which is a comma separated string.get_expired_cookies has been changed from accepting list-of-tuple headers to string of comma separated strings.
I don't see of a way to access more-or-less raw cookie data from requests in any way but parsing them out of the comma separated string without resorting to using protected members. Cookie jar has more formatted cookies but they are stored in a lossy way.
With all this in mind, to keep tests as lists the cookie splitting logic can be moved to outside of
get_expired_cookies()
, but I'm not sure if this is what you had in mind by your comment.Thanks
--IAS
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.
@IlyaSukhanov Is it an option to add a test case to ensure
response.headers.get('Set-Cookie', '')
returns a string with concatenated cookies? It will help us prevent potential regressions for upcomingrequests
versions and to be more comfortable with those changes too :)After that, we will be good to merge.
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.
Added test to ensure that multiple cookies get extracted correctly.
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.
Looks like cicd checks is failing. Is there a tried approach for starting a local http server to test against?
The way it's erroring out it's not clear what the issue is. Few of suspicions I have:
Without knowing what the error is or much experience with GitHub check infrastructure. My only idea is to add a retry functionality in test, to call httpie multiple time until hopefully the background server becomes responsive.
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 was referring to the incorrect usage of tuples for what by nature is a list.
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 think I see what you mean. But the cookie values are not tuples its a string that is line wrapped, and thus grouped using parenthesis.
Tuple:
String:
Note how in this example and in the pr the lines are not separated by comma which is required in tuple definition.
I hope this clears up any confusion.
If project has different style requirement for line wrapping long strings, please let me know and I'll make this change conform to that.
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.
@IlyaSukhanov right, I see it now! Yeah, this style of string-wrapping is the preferred one, but in this context, next to a bunch of tuples, it’s easy to misread it (🙋♂️), so I’d vote for using a different style here.