Skip to content

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

Closed
makermelissa opened this issue Oct 9, 2020 · 10 comments
Closed

Breaking change in 1.7.0: Header keys are now capitalized #40

makermelissa opened this issue Oct 9, 2020 · 10 comments

Comments

@makermelissa
Copy link
Collaborator

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:

title = str(title.lower(), "utf-8")

in #31

I'm not sure if we want to put it back or just slowly change everything that broke.

@tannewt
Copy link
Member

tannewt commented Oct 9, 2020

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.

@makermelissa
Copy link
Collaborator Author

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.

@makermelissa
Copy link
Collaborator Author

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?

@dhalbert
Copy link
Contributor

dhalbert commented Oct 9, 2020

Regular requests has a CaseInsensitiveDict class for exactly this reason:
https://github.com/psf/requests/blob/3bb13f8fbb9d4ed1a20bd33495cdc087eb062ca0/requests/structures.py#L37

@tannewt
Copy link
Member

tannewt commented Oct 9, 2020

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.

Calling .lower() is copying the string. I'm really trying to avoid that.

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?

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.

@makermelissa
Copy link
Collaborator Author

Ok cool. Thanks. I just wanted to make sure I understood.

@justmobilize
Copy link
Collaborator

Is this still valid? Some header fixes were recently merged in, and if this still exists, I'm happy to work on it.

@justmobilize
Copy link
Collaborator

@makermelissa and @tannewt with some other bugs, I cleaned up how headers are handled. Is there any more work there I can do?

@tannewt
Copy link
Member

tannewt commented Mar 18, 2024

I have no idea. I think you know better than I do.

@justmobilize
Copy link
Collaborator

I feel this is good to close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants