Skip to content

move default list serializer into an over-loadable parameter #5393

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

Closed

Conversation

claytondaley
Copy link

Description

Fixes #5391.

@xordoquy
Copy link
Collaborator

xordoquy commented Sep 4, 2017

I'm not sure I get the PR here.
Can't this be achieved with:

class SomeSerializer():
    class Meta:
        list_serializer_class = SomeOtherSerializer

@claytondaley
Copy link
Author

claytondaley commented Sep 4, 2017

For an end-user class yes. But drf-hal-json introduces a HalModelSerializer that will be used by end-user classes. If HalModelSerializer used a Meta class, users would constantly blow it away if they didn't know to inherit the Meta class as well:

class MySerializer(HalModelSerializer):
   class Meta:
        # local configurations (missing ListSerializer configuration)

FWIW I didn't realize that ListSerializer is defined below this class. I would normally reorganize the order of my classes, but I'm not sure if there's anything better to reduce rebase/history hassles.

@claytondaley
Copy link
Author

The current workaround avoids this complexity by injecting the value. If DRF makes this change, it can be replaced by a one-line configuration of the Serializer.

@carltongibson
Copy link
Collaborator

So, this breaks all the tests.

Given that ListSerializer is a BaseSerializer subclass it’s going to need something more sophisticated.

At that point I’m thinking injecting into the Meta class isn’t actually that bad. It’s a good workaround and avoids adding complexity for a specialist case.

I’m going to close this as is. Happy to reopen if there’s a simple enough solution.

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.

3 participants