-
Notifications
You must be signed in to change notification settings - Fork 35
Breaking change in 1.7.0: Header keys are now capitalized #40
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
Comments
Looks like regular requests is case-insensitive: https://requests.readthedocs.io/en/master/user/quickstart/#response-headers Maybe we need a wrapper object to do it that way. I removed the title.lower() because it allocates a second copy of the string when one already exists. |
We could have added .lower() onto the title as it is put into the dict. To handle the differences in the PyPortal, I submitted a PR that creates a new dict and just copies the headers with lower case. This way both older and newer versions of this library should work with it. |
By wrapper object, are you suggesting adding a function like get_header() that would accept lowercase and uppercase or maybe a property that lowercases it on the fly or something else? |
Regular |
Calling
No, I'm thinking along the lines of what Dan pointed out. Create an object that acts like a dictionary and does a case insensitive search on each access. That way you only pay the cost on the headers you care about. |
Ok cool. Thanks. I just wanted to make sure I understood. |
Is this still valid? Some header fixes were recently merged in, and if this still exists, I'm happy to work on it. |
@makermelissa and @tannewt with some other bugs, I cleaned up how headers are handled. Is there any more work there I can do? |
I have no idea. I think you know better than I do. |
I feel this is good to close |
I noticed between 1.6.0 and 1.7.0, headers such as headers['content-length'] are not headers['Content-Length'], so anything that uses the head keys will need to be updated.
This issue is more of a breaking change alert due to the removal of this line:
in #31
I'm not sure if we want to put it back or just slowly change everything that broke.
The text was updated successfully, but these errors were encountered: