Add separators encoding parameter#557
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #557 +/- ##
==========================================
- Coverage 91.84% 91.53% -0.32%
==========================================
Files 6 6
Lines 1852 1902 +50
==========================================
+ Hits 1701 1741 +40
- Misses 151 161 +10 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| oseparatorsItem = PyTuple_GetItem(oseparators, 0); | ||
| if (PyErr_Occurred()) | ||
| { | ||
| return NULL; |
There was a problem hiding this comment.
Surely this line isn't reachable if the check above has already confirmed that there are two separators?
There was a problem hiding this comment.
You're right that it's currently impossible to hit this given CPython's implementation of PyTuple_GetItem. But there's no guarantee that it couldn't fail for reasons unrelated to the index not matching the tuple's size...
The check should probably be if (!oseparatorsItem) (or if (oseparatorsItem == NULL), existing code is inconsistent) though, which should perform better as well, although that's unlikely to matter in the setup function.
| ((True, 0), TypeError), | ||
| ((",", True), TypeError), | ||
| ((True, ":"), TypeError), | ||
| ((b",", b":"), TypeError), |
There was a problem hiding this comment.
Can we have a malformed UTF8 string in here too? Coverage is moaning about those lines + the ERROR goto never getting tested.
There was a problem hiding this comment.
No, we can't, for the same reason that #537 can't be tested for: we pass through surrogates now, so the conversion from PyUnicode to char* can never fail, practically speaking. Testing those encoding failures will require the C API instrumentation mentioned in that issue, which does not exist yet apart from a basic PoC.
|
Thank you for finally implementing (and publishing) the separators. Could you please add this very important information to your documentation? This is very important, since json.dumps is also often used to generate signatures for API requests and there the missing spaces causes trouble and it took me ages to find out why the signature is invalid now (when I used ujson instead of json). |
5 participants
Closes #283