Skip to content
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

Test Serializer exclude for declared fields #5599

Merged
merged 2 commits into from
Nov 20, 2017

Conversation

rpkilby
Copy link
Member

@rpkilby rpkilby commented Nov 15, 2017

I'm not particularly inclined towards the change in behavior, but it was fairly trivial to implement.

Thoughts @xordoquy, @carltongibson?

@rpkilby rpkilby added this to the v3.8 milestone Nov 15, 2017
@xordoquy
Copy link
Collaborator

@rpkilby as said on #5596, I'm not in favor of this change. We could make the documentation a bit more explicit though.

@rpkilby
Copy link
Member Author

rpkilby commented Nov 15, 2017

@xordoquy - agreed, I just whipped it up given how simple of a change it was. I've added another commit that raises an AssertionError instead.

@carltongibson
Copy link
Collaborator

For me, an assert here is fine. (Even “good” perhaps 🙂) I’m -1 on supporting the usage from #5596, as per discussion there.

It’s my birthday 🍰 today so I’ll leave you folks to decide what to do.

@rpkilby rpkilby requested a review from xordoquy November 16, 2017 13:35
@rpkilby rpkilby force-pushed the exclude-declared-fields branch from 3578ba6 to 4e7e728 Compare November 20, 2017 01:16
Copy link
Collaborator

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

OK, this is fine. I'm sure it will help someone. Let's have it.

@carltongibson carltongibson modified the milestones: v3.8, v3.7.4 Nov 20, 2017
@carltongibson carltongibson merged commit a3df1c1 into encode:master Nov 20, 2017
@rpkilby rpkilby deleted the exclude-declared-fields branch November 20, 2017 08:55
@virusdefender
Copy link
Contributor

virusdefender commented Dec 1, 2017

It's great!

I have the same issue. When you redeclared some field, it won't be filtered.

georgebabey added a commit to openedx-unsupported/edx-analytics-data-api that referenced this pull request Aug 29, 2018
Unpinned a number of requirements so they would be upgraded as part of
`make upgrade` as recommended by OEP-18. In particular the edx-enterprise-data
django app was not pinning edx-drf-extensions which resulted in 2 different
versions of DRF in the requirements (one for enterprise-data and one for this
IDA).

Upgrading DRF resulting in an assertion failure that was added in DRF 3.7.4
```
Cannot both declare the field 'created' and include it in the CourseProgramMetadataSerializer
'exclude' option. Remove the field or, if inherited from a parent serializer, disable with `created = None`.
```

encode/django-rest-framework#5599

Because the created date was being excluded, it is not needed in the CourseProgramMetadataSerializer and
has been removed.
pchiquet pushed a commit to pchiquet/django-rest-framework that referenced this pull request Nov 17, 2020
* Test current behavior of exclude+declared field

* Assert declared fields are not present in exclude
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ModelSerializer exclude won't work well when field duplicate declear in Serializer
4 participants