-
Notifications
You must be signed in to change notification settings - Fork 62
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
Add config option to specify json float serialization precision #905
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,10 +4,26 @@ | |
from asyncio import StreamReader, StreamWriter | ||
|
||
import server.metrics as metrics | ||
from server.config import config | ||
|
||
from ..asyncio_extensions import synchronizedmethod | ||
|
||
json_encoder = json.JSONEncoder(separators=(",", ":")) | ||
|
||
class CustomJSONEncoder(json.JSONEncoder): | ||
# taken from https://stackoverflow.com/a/53798633 | ||
def encode(self, o): | ||
def round_floats(o): | ||
if isinstance(o, float): | ||
return round(o, config.JSON_FLOAT_MAX_DIGITS) | ||
if isinstance(o, dict): | ||
return {k: round_floats(v) for k, v in o.items()} | ||
if isinstance(o, (list, tuple)): | ||
return [round_floats(x) for x in o] | ||
return o | ||
return super().encode(round_floats(o)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should add a config option to disable this feature just in case it turns out to be too costly in production. Although, I think even if it doubles the json encoding time that will still be fine judging by the profiling information I've collected from the server in the past. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, sure, what would you call it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should mocking be also conditional in tests? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it's too important to have tests for the config values. They're generally simple enough that it doesn't matter |
||
|
||
|
||
json_encoder = CustomJSONEncoder(separators=(",", ":")) | ||
|
||
|
||
class DisconnectedError(ConnectionError): | ||
|
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 like this way the best out of all the ones proposed so far. Have you measured the performance difference for this one? I imagine it would be noticeable especially for dicts since it makes a copy of the entire data structure, but maybe it's not too bad.
I think the other way that would have merit would be to make a wrapper type around
float
and usedefault
to format it. That would require us to explicitly set the float rounding everywhere, but it would also give more control if we wanted to round rating to 3 or 4 places but timestamps only to 2.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.
Oh sorry, I see this is the 'round floats' approach you mentioned in your other comment.
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 don't quite understand what do you mean by a wrapper, because if it requires us to call this wrapper every time we do some math, then why don't just use
round
?Rewriting representation of the float won't help, the C code will operate with value and encode all the digits
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.
Something similar to this, but using our own class instead of Decimal: https://stackoverflow.com/a/3885198
And then in the
to_dict
method you'd have to wrap the floats in this class: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.
Yeah, I can make a class, but I think it is unnecessary complication, because we already have built-in
round
function which effectively does the same thingThere 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.
Like, we have 2 options:
encode
method to change encoding of all floatsto_dict
method where we will round every float with its own precision (the "more control" mentioned above)