utils: allow ujson for requests calls if it can be imported#102
utils: allow ujson for requests calls if it can be imported#102RussTorres merged 2 commits intodevelopfrom
Conversation
|
For testing, I think we should treat this like we're treating scipy in the transform module -- have it as a test dependency and toggle its importability -- but I'm not sure which tests make sense to run with/without ujson since we don't have explicit integration tests for the utility module. |
Codecov Report
@@ Coverage Diff @@
## develop #102 +/- ##
===========================================
+ Coverage 93.86% 93.88% +0.02%
===========================================
Files 16 16
Lines 1662 1668 +6
===========================================
+ Hits 1560 1566 +6
Misses 102 102
Continue to review full report at Codecov.
|
|
in travis we could make a matrix of environment variables, and have the import be conditional on an environment variable, and it would test it with the variable both ways. Or something akin to that.. |
|
|
||
| # use ujson if installed for faster json | ||
| try: | ||
| import ujson as requests_json |
There was a problem hiding this comment.
you put this in here but didn't actually make use of it yet.
There was a problem hiding this comment.
right -- our test environment doesn't have ujson yet, but if it did we would use it as the default json implementation for all the requests json handling. I suppose we could just assume that the standard library json interface works if ujson works as well as checking to make sure that the defaulting works.
There was a problem hiding this comment.
can we just put it in test_requirements?
overwrites default json handler for requests model with ujson if it is importable.
in testing cjson, simplejson, standard json, and ujson, ujson was the clear winner in all trials where there was a difference -- this seems to confirm what the internet has to say about the issue as well. This change should resolve #78 to a reasonable degree while not altering the code in the library.