Skip to content
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

Unicodify the header name (key) as well as value #1346

Merged
merged 1 commit into from Nov 22, 2018
Merged

Conversation

@pgjones
Copy link
Member

@pgjones pgjones commented Aug 25, 2018

In Python 2 using a Headers object as so,

h = Headers()
h[b'X-Foo'] = b'something'

gives

X-Foo: something

in the actual HTTP message, whereas in Python 3 it is

b'X-Foo': something

this is because only the value is currently unicodified. This change
ensures the header name (key) is also unicodified as well therefore
matching the Python 2 usage and the expected usage.

See also pallets/flask#1986

This also removes a regression test for unicode header keys that is
not required. It was recognised in pallets/flask#758 as resulting in a HTTP
message containing,

u'X-Foo': something

however this behaviour was fixed in f3435a3 introducing the test, broken
in 6049a4f and then comprehensively fixed in db00dfb. Therefore with the
last commit and tests as added the regression test is not required.

In Python 2 using a Headers object as so,

    h = Headers()
    h[b'X-Foo'] = b'something'

gives

    X-Foo: something

in the actual HTTP message, whereas in Python 3 it is

    b'X-Foo': something

this is because only the value is currently unicodified. This change
ensures the header name (key) is also unicodified as well therefore
matching the Python 2 usage and the expected usage.

This also removes a regression test for unicode header keys that is
not required. It was recognised in flask#758 as resulting in a HTTP
message containing,

    u'X-Foo': something

however this behaviour was fixed in
f3435a3 introducing the test, broken
in 6049a4f and then comprehensively
fixed in db00dfb. Therefore with the
last commit and tests as added the regression test is not required.
@davidism davidism merged commit 962746d into pallets:master Nov 22, 2018
1 check passed
@pgjones pgjones deleted the headers branch Jan 19, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants