-
Notifications
You must be signed in to change notification settings - Fork 363
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
If an object has a __json__ method, use it when encoding #157
Conversation
It should return a raw JSON string which will be directly included in the resulting JSON when encoding.
Fixed Travis CI tests. |
+1 for this feature |
+1 |
7 similar comments
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 |
+1 and bump. @esnme? |
+1 |
Thanks, Looks good. The only problem I could see with this PR is that it means that |
Yes. I do not think we should try to verify that. This is your responsibility. Luckily, JSON is nicely recursive, so if the output of |
If an object has a __json__ method, use it when encoding
Agree, thanks again. |
@@ -579,7 +592,7 @@ void Object_beginTypeContext (JSOBJ _obj, JSONTypeContext *tc) | |||
return; | |||
} | |||
else | |||
if (PyString_Check(obj)) | |||
if (PyString_Check(obj) && !PyObject_HasAttrString(obj, "__json__")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @mitar, What's the use-case that warranted this particular change? It looks like this is a performance regression.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So that one can have strings with __json__
method. If a string does not have __json__
method, we go by the normal flow, if it has, we use it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far I did follow. But not why one would need it. What's the actual use-case for that? Furthermore why would only the string type have a __json__
method but not the other types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leaning towards removing this and thus limiting the __json__
functionality to objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the idea is a bit that you can have a magical string, which behaves in one way when used as a string, but another (optimized) when used as a JSON. Specifically, you can have a JSON serialization, which is a string, but also when serialized, it is not converted to a string (so no double encoding), but output directly, because it already contains a proper JSON serialization.
Example use for me is how to properly serialize GeoJSON Django fields. I create a GeoJSON string class, and then returned it as an optimized value. In this way existing codebase which expects strings work well, but when the time for serialization to JSON comes, one can quickly render JSON, because it is already there. Moreover, if you serialize to XML, instead, then the string is used as string, so you get embedded JSON inside XML, which is also good in the case of GeoJSON.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the explanation. However this is too much of an edge-case to justify the performance penalty as it stands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious, what was the % of the performance hit in your tests because of that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was the cause if this one: #223
It should return a raw JSON string which will be directly included in the resulting JSON when encoding.
Similar to #130, but it allows an object to return already encoded string which is then included as it is in the output. This allows one to pass embed existing JSON strings inside a larger structure, without a need to deserialize and serialize again.