Skip to content

Use serializers.Serializer to mixin with plain Serializer #11

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 11, 2017
Merged

Use serializers.Serializer to mixin with plain Serializer #11

merged 3 commits into from
Oct 11, 2017

Conversation

phillbaker
Copy link
Contributor

So that serializers which don't inherit from ModelSerializer can use this functionality.

In retrospect - this would be a breaking change. Any thoughts on how to approach this gracefully?

An example:

# using a plain serializer
class FooSerializerV3(BaseVersioningSerializer):
    test_field_two = serializers.CharField()

# using model serializer
class TestSerializerV3(serializers.ModelSerializer, BaseVersioningSerializer):
    transform_base = 'tests.test_transforms.TestModelTransform'

    class Meta:
        model = TestModelV3
        fields = (
            'test_field_two',
            'test_field_three',
            'test_field_four',
            'test_field_five',
            'new_test_field',
            'new_related_object_id_list',
        )

@mrhwick
Copy link
Owner

mrhwick commented Sep 7, 2017

I like your idea of using multiple inheritance here, and I don't think it will incur any obvious incompatibilities.

Have you tried fixing those broken tests by making sure the test serializers are inheriting from serializers.ModelSerializer now that it isn't being inherited by association from BaseVersioningSerializer?

@phillbaker
Copy link
Contributor Author

Thanks for the feedback! I'll fix up those tests and any documentation that might also need to be updated.

@mrhwick
Copy link
Owner

mrhwick commented Sep 8, 2017 •
edited
Loading

You could also add in a smaller set of tests that gives us the same guarantees about basic serializers (inheriting only from Serializer) as we currently have with the full model serializer tests. That would make me feel comfortable about my assumption that we wouldn't be causing problems for those that use that newly available configuration.

@phillbaker phillbaker force-pushed the patch-1 branch 3 times, most recently from 7ebf137 to b0678b2 Compare September 11, 2017 13:54
@phillbaker phillbaker force-pushed the patch-1 branch 2 times, most recently from 2a75ba3 to cc3a95d Compare September 11, 2017 15:04
@phillbaker
Allow mixins with plain Serializer.
b0a8bb4 
For example:

```
class FooSerializerV3(BaseVersioningSerializer, serializers.Serializer):
    test_field_two = serializers.CharField()

class TestSerializerV3(BaseVersioningSerializer, serializers.ModelSerializer):
    transform_base = 'tests.test_transforms.TestModelTransform'

    class Meta:
        model = TestModelV3
        fields = (
            'test_field_two',
            'test_field_three',
            'test_field_four',
            'test_field_five',
            'new_test_field',
            'new_related_object_id_list',
        )
```
@phillbaker
Copy link
Contributor Author

Updated to follow your suggestions - I had to add a couple of non-related commits to get this green on travis.

@mrhwick
Copy link
Owner

mrhwick commented Sep 13, 2017

I'll be able to take a look at this soon, and should be able to get it merged in ASAP after review. Thanks for submitting a PR!

@mrhwick mrhwick merged commit 23f8a00 into mrhwick:dev Oct 11, 2017
@mrhwick
Copy link
Owner

mrhwick commented Oct 11, 2017

@phillbaker Apologies for taking so long to merge this. Thank you for the PR, this was excellent work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrhwick mrhwick mrhwick approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@phillbaker @mrhwick