-
Notifications
You must be signed in to change notification settings - Fork 6k
Drop support for ujson
#3037
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
Drop support for ujson
#3037
Conversation
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.
Hey! Looks like you edited README.rst or README_RAW.rst. I'm just a friendly reminder to apply relevant changes to both of those files :)
|
@Bibo-Joshi Thanks! This sums up our conversation very well. |
Poolitzer
left a 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.
Actually found a mistake \o/
|
I would definitely appreciate an easy way to customize the json backend as a fan of ujson. I would like to be able to keep using it in my Telegram bots. |
|
Thanks for reaching out @MiguelX413. I would like to have this change published in a release before making a decision on this topic, so that we can get a better impression of the impact of this PR on the community. I hope that you can understand my reasoning. Would you mind opening a feature request issue so that we don't forget about this? |
That makes sense to me. @Bibo-Joshi
Sure thing. |
Let me try to wrap up what lead us here:
ujsontoorjsonas optional 3rd party json library, because the latter has serveral benefitsorjson.dumpsreturns bytes instead of a stringHence, this PR removes support for
ujson.We don't really know how many users are actually using the
ujsonfeature currently and we also don't have any benchmarks on howujsonactually affects the performance. If users do request the option to use a custom json library, there are a few things that we can do and that we need to consider. I'm writing them down below in some detail for future reference.ujsoncurrently used, i.e. where could users want to use a 3rd party library instead?DictPersisence: This is no problem.BasePersistenceis already an ABC, so any user can go ahead and implement a custom version ofDictPersistencewith a 3rd party json libraryWebhookHandler: No problem. Users can implement a custom webhook solution that uses a 3rd party json libTelegramObject.to_json: No problem. This method is not actually used anywhere.BaseRequest._parse_json_payloada public method. Then custom implementations can override it to use a 3rd party json lib.do_requestjust do something like{name: my_json_lib.dumps(value) for name, value in request_data.parameters.items()}. However, the big performance advantage of some json libraries comes from the fact that they can readily json-dump a larger variety of objects, i.e. not only lists, dicts, strings & numbers but also e.g.datetimeanddataclassobjects. So a user might want to skip theTelegramObject.to_dict()step. To support this, we could consider to add a propertRequestData.raw_parameters(or similar naming) through which we expose exactly those values.