-
Notifications
You must be signed in to change notification settings - Fork 299
Load included and related serializers in meta class #926
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
Load included and related serializers in meta class #926
Conversation
Codecov Report
@@ Coverage Diff @@ ## master #926 +/- ## ======================================= Coverage 96.81% 96.82% ======================================= Files 65 65 Lines 3894 3904 +10 ======================================= + Hits 3770 3780 +10 Misses 124 124
Continue to review full report at Codecov.
|
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 working on this. See my inline comments and do not forget to write a changelog entry.
Changes done.
But I'm not sure what to do with "self" and inline self-related serializers.
Ping?
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 haven't forgotten about this PR but haven't really had time to look at it closely.
Seems some comments were not addressed but somehow through outdating were resolved by GitHub directly. So I moved those comment to the right code line.
Sorry if this has caused lots of notifications. The conversations which are still open are the issues which still need to be addressed. Thanks.
I'm sorry but I can't work on this anymore. I tried an approach where I set included_serializers and related_serializers to None if not provided, but it resulted in a bit more complicated code, you'll find it before applying the commit "Simplify caching included serializers".
@SafaAlfulaij Thanks for having worked on this. I made the last minor changes and this PR should be ready now and be merged once CI is green.
@sliverc this is a pretty neat performance boost, what's the plan for the next release?
@jarekwg I would not overestimate this change in terms of performance because it's just few imports less. This is mainly a API cleanup so included serializers can be simply retrieved by serializer.included_serializers instead of a utility method.
In terms of next release I assume we will do one once Django 4.0 is released (according to release notes expected in December).
besides django 4.0 compat new release is needed in drf as well.
Uh oh!
There was an error while loading. Please reload this page.
Fixes #858
Description of the Change
As mentioned in the issue, this caches
included_serializersandrelated_serializersin the dictionary to ease the process of retrieving in various places.This approach is also used by DRF for binding fields: https://github.com/encode/django-rest-framework/blob/71e6c30034a1dd35a39ca74f86c371713e762c79/rest_framework/utils/serializer_helpers.py#L137
Checklist
(削除) added (削除ここまで)updatedCHANGELOG.mdupdated (only for user relevant changes)AUTHORS