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

Escape \u2028 and \u2029 in JSON output. #2195

Merged
merged 1 commit into from Dec 5, 2014

Conversation

tomchristie
Copy link
Member

@tomchristie tomchristie commented Dec 3, 2014

Closes #2169.

@jpadilla
Copy link
Member

jpadilla commented Dec 5, 2014

LGTM but I'm thinking we should just fix this under the JSONPRenderer instead.

@tomchristie
Copy link
Member Author

tomchristie commented Dec 5, 2014

Perhaps. I'm expecting we'll also have folks using JSONRenderer and putting the results into a template as a javascript variable.

@jpadilla
Copy link
Member

jpadilla commented Dec 5, 2014

Makes sense. Having it under the JSONRenderer covers both cases so that's good.

@tomchristie
Copy link
Member Author

tomchristie commented Dec 5, 2014

Don't feel like there's any great solutions here - not wild about potential for it to be an unneeded minor performance hit for the folks who don't need it, but there's no real way round it, so. (And presumably not actually an issue, but still meh)

@jpadilla
Copy link
Member

jpadilla commented Dec 5, 2014

I still haven't seen any further reports about it though, so I think its totally valid we consider actually adding this vs perhaps leaving as is for now. Also, now that I think of it, might be interesting to allow users to swap json.dumps on the renderer for something like simplejson.dumps instead.

@tomchristie
Copy link
Member Author

tomchristie commented Dec 5, 2014

I think its totally valid we consider actually adding this vs perhaps leaving as is for now.

Sorry, wasn't clear what you synopsis was on this?

@tomchristie tomchristie closed this Dec 5, 2014
@tomchristie tomchristie reopened this Dec 5, 2014
tomchristie added a commit that referenced this pull request Dec 5, 2014
…029-json

Escape \u2028 and \u2029 in JSON output.
@tomchristie tomchristie merged commit de4ef6e into master Dec 5, 2014
@tomchristie tomchristie deleted the tomchristie-escape-u2028-u2029-json branch Dec 5, 2014
@jpadilla
Copy link
Member

jpadilla commented Dec 5, 2014

@tomchristie sorry for not being more clear. What I mean was that if we weren't happy with adding this and hasn't been a recurring brought-up issue we could just leave it out for now specially if you're worried about any potential hits for the 99.99% who haven't had this problem.

@tomchristie
Copy link
Member Author

tomchristie commented Dec 5, 2014

Just fixing the damn thing.

JSON spec authors oughta get their shit together and put a single para ammendment in recommending that those 2 chars be escaped and.
Python team oughta get their shit together and fix it in the json pacakge directly.

:-|

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

Successfully merging this pull request may close these issues.

JSONP renderer fails to escape U+2028 and U+2029
2 participants