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

Use ujson if its available #78

Closed
wants to merge 7 commits into from
Closed

Conversation

joshowen
Copy link
Contributor

If you have it, might as well monkey patch botocore.vendored.requests to use it too, but thats out of scope.

https://github.com/kennethreitz/requests/issues/1595

@joshowen joshowen mentioned this pull request Sep 23, 2015
6 tasks
@danielhochman
Copy link
Contributor

This area of the code will pay the biggest dividend in terms of switching out the parser. Also not calling .json() twice in that block would be a good idea.

https://github.com/jlafon/PynamoDB/blob/33eb91a40ee252525871fa1ce0e47cf6b002ad17/pynamodb/connection/base.py#L238

@joshowen
Copy link
Contributor Author

That is where the monkey patching mentioned above comes in. And json isn't called twice there, the first branch raises.

@danielhochman
Copy link
Contributor

we don't need to patch though, why not call json.loads(request.text)?

@danielhochman
Copy link
Contributor

+1 from me

@joshowen
Copy link
Contributor Author

@jlafon any thoughts

Josh Owen added 2 commits January 22, 2016 10:11
@joshowen
Copy link
Contributor Author

Thoughts on merging this in? I'm happy to update the branch. We've been using this in production for a while.

@danielhochman
Copy link
Contributor

@joshowen definitely. let's take advantage of the new global config introduced in 1.6.0 and allow you to globally patch in whatever json parser you want, defaulting to the built-in json.

let me know if you have any questions.

@garrettheel garrettheel closed this May 2, 2017
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