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

utf-8 instead of latin-1 encoding #1236

Closed
wants to merge 1 commit into from

Conversation

TomaszSzyborski
Copy link

Using utf-8 instead of latin-1 encoding as other than latin-based languages e.g. Polish using in various places e.g. cookies - produced 500 Internal Server Error.
Chaning encoding fixes this issue.

Comment on lines +66 to +67
assert "123" in response.text
assert response.text.endswith("\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
assert "123" in response.text
assert response.text.endswith("\n")
assert response.text == "123\n"

@JayH5
Copy link
Member

JayH5 commented Jul 4, 2021

I think this change would be better as a discussion topic, as the pull request template encourages you to do.

You also need to be specific about what (cookies/headers/query strings) you want to change the encoding for, and you'll need to back up your claim with some kind of HTTP standard. For example, RFC 7230 says that headers should be ISO-8859-1 (i.e. latin1), hence why Starlette works that way.

It is important that Starlette supports languages such as Polish, but please start this out with a discussion topic where you point out where you're facing incompatibility with character sets and we can work from there. I'm going to close this PR because this does not follow our contributing guidelines.

@JayH5 JayH5 closed this Jul 4, 2021
@graingert
Copy link
Member

@graingert
Copy link
Member

here's a discussion: #1238

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

Successfully merging this pull request may close these issues.

3 participants