-
-
Notifications
You must be signed in to change notification settings - Fork 9.2k
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
New HTTPHeaderDict class for response headers with multiple header value support #4282
Conversation
…with the same name are compiled into a single value.
Codecov Report
@@ Coverage Diff @@
## master #4282 +/- ##
==========================================
+ Coverage 88.66% 88.93% +0.26%
==========================================
Files 18 18
Lines 2021 2079 +58
==========================================
+ Hits 1792 1849 +57
- Misses 229 230 +1
Continue to review full report at Codecov.
|
I generally like this. This should be a PR against the |
I also wouldn't be opposed to the name |
I haven't checked, but I'd like this API to be compatible with Flask/Werkzeug's MultiDict, if possible. |
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.
So yeah, requested changes:
- Make this against the
proposed/3.0.0
branch, instead ofmaster
: - Rename to
CaseInsensitiveMultiDict
- Use same API as Flask/Werkzeug's
MultiDict
.
In general though, this looks real solid! ✨🍰✨ |
Good job with the tests too 🍪 |
You get a cookie. |
I also really like that this is a subclass of |
With regards to MultiDict in werkzeug, it uses I did have concerns about the lists being modified outside of HTTPHeaderDict (which tries to enforce that only string types are present). So it's either a case of::
|
Renaming The main intent was to keep the exact same behaviour that we had in requests for response header processing, but to handle the concatenation of values ourselves (rather than letting urllib3 to do it for us). The concatenation of values is specific to HTTP headers (comma separation). I could refactor part of the code and extract the |
I think what you have is good, then, unless we cast to lists. |
Which seems silly to me. |
I just wanted to provide a very familiar API to those who have used similar APIs before. |
I've just always liked the name |
…stent with other similar implementations.
OK, I've renamed the methods to I was going to try and extract some of the code into a I've made sure that this class is entirely backwards compatible with what we were using before - I wrote some of the tests using |
@the-allanc definitely :) P.S. good work! |
In that case, I'm closing this pull request in favour of #4290. |
I personally wanted something like this while writing my own Requests-related library, but I can see other people also have wanted something like this - #3957 and #4214 both mention it in terms of handling
Set-Cookie
headers.I've written an subclass implementation of
CaseInsensitiveDict
which is compatible with all the current usage (according to the tests), but adds new methods for clients that want to deal directly with unjoined headers. I've also added quite a number of tests and made slight changes to the documentation in relation to it.