Skip to content
This repository has been archived by the owner on Mar 28, 2019. It is now read-only.

Upgrade ujson to 1.35 #537

Merged
merged 4 commits into from
Jan 22, 2016
Merged

Upgrade ujson to 1.35 #537

merged 4 commits into from
Jan 22, 2016

Conversation

Natim
Copy link
Contributor

@Natim Natim commented Nov 2, 2015

@Natim
Copy link
Contributor Author

Natim commented Nov 2, 2015

AFAIK it doesn't seems to fix the problem.

@Natim
Copy link
Contributor Author

Natim commented Nov 2, 2015

Or maybe it is because of missing , escape_forward_slashes=False ?

@Natim
Copy link
Contributor Author

Natim commented Nov 2, 2015

I added the option but I have no idea if it will actually works with normal json.

@Natim
Copy link
Contributor Author

Natim commented Nov 2, 2015

TypeError: __init__() got an unexpected keyword argument 'escape_forward_slashes'; uid=None; errno=None; agent=curl/7.35.0; authn_type=None

@leplatrem any idea of how to handle this?

@@ -50,7 +50,8 @@ def setup_json_serializer(config):
requests.models.json = utils.json

# Override json renderer using ujson
renderer = JSONRenderer(serializer=lambda v, **kw: utils.json.dumps(v))
renderer = JSONRenderer(serializer=lambda v, **kw: utils.json.dumps(
v, escape_forward_slashes=False))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The escape_forward_slashes only works with ultrajson.

Copy link
Contributor

Choose a reason for hiding this comment

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

The authors could not publish the version with the escape option on Pypi yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The authors could not publish the version with the escape option on Pypi yet

The ujson people couldn't but a fork ultrajson was made available with it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The escape_forward_slashes only works with ultrajson.

What if we do renderer = JSONRenderer(serializer=utils.json_serializer) ?

@leplatrem
Copy link
Contributor

See ultrajson/ultrajson#185

@Natim
Copy link
Contributor Author

Natim commented Nov 2, 2015

@leplatrem well it doesn't really change the matter on this PR but ok I understand that.

@Natim
Copy link
Contributor Author

Natim commented Nov 3, 2015

With my last commit I've got two test failing on this kind of JSON:

u'{"responses":[{"status":201,"path":"/v0/mushrooms/e93e0961-0e09-4d8f-9ee0-8b05cdf18f41","body":{"data":{"last_modified":1446547536607,"name":"Trompette de la mort","id":"e93e0961-0e09-4d8f-9ee0-8b05cdf18f41"}},"headers":{"Last-Modified":"Tue, 03 Nov 2015 10:45:36 GMT","Content-Length":"114","ETag":""1446547536607"","Content-Type":"application/json; charset=UTF-8","Access-Control-Expose-Headers":"Retry-After, Content-Length, Alert, Backoff"}}]}'

except ImportError: # pragma: no cover
import json # NOQA

def json_serializer(v, **kw):
return json.dumps(v)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'm not 100% sure that it is better, but you can do json_serializer = json.dumps

import ultrajson as json # NOQA

def json_serializer(v, **kw):
return json.dumps(v, escape_forward_slashes=False)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I'm not 100% sure that it is better, but you could do json_serializer = functools.partial(json.dumps, escape_forward_slashes=False)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Telepathy for the win 👍

@Natim
Copy link
Contributor Author

Natim commented Nov 3, 2015

Well it is even worse actually :(

@leplatrem
Copy link
Contributor

I tried to update this, but found a problem in the JSON serialization of batch responses:

capture d ecran de 2015-11-03 17 19 12

It means that " are not escaped within strings :/

@Natim
Copy link
Contributor Author

Natim commented Nov 3, 2015

It means that " are not escaped within strings :/

Yes it is what I also noticed, do you know if it is a bug with your ultrajson fix?

@leplatrem
Copy link
Contributor

I guess it is a known bug, with a pending PR to fix it.

>>> import ujson
>>> ujson.dumps('"')
'"\\""'

>>> import json
>>> json.dumps('"')
'"\\""'

>>> ujson.dumps('"', escape_forward_slashes=False)
'"""'

@leplatrem
Copy link
Contributor

I updated this. Needs rebase.

@leplatrem leplatrem changed the title ujson was not released since 3 years. Use ultrajson instead. Upgrade ujson to 1.35 Jan 21, 2016
@Natim
Copy link
Contributor Author

Natim commented Jan 21, 2016

Let me rebase then 👍

@leplatrem
Copy link
Contributor

@Natim final r+ ?

@Natim
Copy link
Contributor Author

Natim commented Jan 21, 2016

GG mat r+

Natim added a commit that referenced this pull request Jan 22, 2016
@Natim Natim merged commit 3297fe5 into master Jan 22, 2016
@Natim Natim deleted the ultra-json-readinglist-224 branch January 22, 2016 09:09
@Natim Natim removed the in progress label Jan 22, 2016
glasserc pushed a commit that referenced this pull request May 20, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants