Skip to content

Conversation

albertisfu
Copy link

@jtrain In this PR, based on your work in #43, I fixed the logic related to serializer level computation for serializers using ListSerializer.

I also refined the implementation, for example, by replacing the while loops in get_source_path and compute_level with a recursive approach.

Additionally, I extended the test suite to cover nested serializers that use ListSerializer, and added a third level of nesting to ensure the logic works correctly across multiple nesting levels.

Let me know what you think.

Thank you!

albertisfu and others added 3 commits July 9, 2025 15:27
Introduces the new code in a separate serializer that inherits from the
existing one so that dynamic field enjoyers can opt into the new
functionality.

However the new field seems to be 100% backwards compatible with the old
one, save for some performance hit since we are doing extra work.
@mlissner
Copy link

Just a heads up, I've assigned somebody else from our team to review this PR, so we should see that review shortly. Hopefully that'll make merging easier, since it usually adds some polish.

I know as a open source maintainer that a PR creates a sense of pressure. I've never had a team submit a PR and then review it, but I assume it'd create an even bigger sense of pressure. I just wanted to write and clarify that that's not our intent at all. The review is helpful for us too, and since we're on a tight timeline, we're vendoring the code into our project anyway, so we get the functionality we need if the PR here isn't desired.

So I hope the review helps, and if the maintainers here like the feature, I hope we can merge it, but if not, no worries. We have the fix we need in our own system already.

I hope that makes sense and thank you to the maintainers for their work on this package! We've used it for years and really appreciate it.

Comment on lines 160 to 162
"""Filter a list of dotted field names down to those relevant at a given
nesting level and prefix.
"""

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@albertisfu I reviewed the new tests and didn't find any dotted fields. We pass the fields to the endpoint as a comma-separated list and use double underscores to represent nested relationships. It’s possible that this docstring is a remnant of a previous attempt to address the issue. could you take a look and update it accordingly?

Copy link

@ERosendo ERosendo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@albertisfu The code looks good overall. Just a small note: my editor flagged some minor styling issues in test_mixins.py, such as spacing between classes and methods within the TestNestedDynamicFieldsMixin class. Aside from that and the docstring comment, everything else looks solid.

@albertisfu
Copy link
Author

Thanks @ERosendo for your review I've applied the suggested changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants