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

Set ensure_ascii=False on json.dumps() for WebSocket.send_json() #2341

Merged
merged 3 commits into from
Dec 1, 2023

Conversation

xiayouran
Copy link
Contributor

Summary

When the data output by the send_json function is in Chinese, it will force Chinese characters to be converted to Unicode, so I fixed this issue.(The most fundamental reason is that the value of the ensure_ascii parameter was not modified when using the json.dumps function)

Checklist

  • [√] I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • [√] I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • [√] I've updated the documentation accordingly.

@Kludex
Copy link
Member

Kludex commented Nov 22, 2023

I think we have some issues about this around. Can we link them here?

@abersheeran
Copy link
Member

abersheeran commented Nov 22, 2023

I'm not sure what problem you encountered that requires adding this option. Sending Chinese should still work fine without using this option. Can you provide a more detailed description of what kind of problem this solves? (Converting Chinese to Unicode encoding is not a problem, it is standard-compliant.)

@xiayouran
Copy link
Contributor Author

xiayouran commented Nov 22, 2023

Yes, from the perspective of the code workflow, there is no problem. I am building a Chinese question and answer system, but when using send_json function sends Chinese characters, my front-end display is Unicode characters instead of Chinese characters, so I want to solve this problem. The problem lies in the fact that the json.dumps function did not set the ensure_ascii parameter to False.

@Kludex Kludex added the hold Don't merge it label Nov 22, 2023
Copy link
Member

@abersheeran abersheeran left a comment

Choose a reason for hiding this comment

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

I verified it, and it was

Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Can we add a test case?

@Kludex Kludex removed the hold Don't merge it label Dec 1, 2023
Copy link
Member

@Kludex Kludex left a comment

Choose a reason for hiding this comment

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

Yeah, according to the spec (https://asgi.readthedocs.io/en/latest/specs/www.html#send-send-event), there was no need to ensure_ascii here. 👍

@Kludex Kludex changed the title fix func send_json to support chinese Set ensure_ascii=False on json.dumps() for WebSocket.send_json() Dec 1, 2023
@Kludex Kludex changed the title Set ensure_ascii=False on json.dumps() for WebSocket.send_json() Set json.dumps(..., ensure_ascii=False) for WebSocket.send_json() Dec 1, 2023
@Kludex Kludex changed the title Set json.dumps(..., ensure_ascii=False) for WebSocket.send_json() Set json.dumps(..., ensure_ascii=False) on WebSocket.send_json() Dec 1, 2023
@Kludex Kludex changed the title Set json.dumps(..., ensure_ascii=False) on WebSocket.send_json() Set ensure_ascii=False on WebSocket.send_json() Dec 1, 2023
@Kludex Kludex changed the title Set ensure_ascii=False on WebSocket.send_json() Set ensure_ascii=False on json.dumps() for WebSocket.send_json() Dec 1, 2023
@Kludex Kludex enabled auto-merge (squash) December 1, 2023 13:22
@Kludex Kludex merged commit a1ee8b4 into encode:master Dec 1, 2023
5 checks passed
@Kludex Kludex mentioned this pull request Dec 1, 2023
4 tasks
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