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

Alias only_fields as fields and exclude_fields as exclude #691

Merged
merged 11 commits into from
Jul 9, 2019

Conversation

jkimbo
Copy link
Member

@jkimbo jkimbo commented Jun 25, 2019

To make it easier for developers who have used Django REST Framework before, change only_fields to fields and exclude_fields to exclude so that it matches the ModelSerializer: https://www.django-rest-framework.org/api-guide/serializers/#specifying-which-fields-to-include

only_fields and exclude_fields will still work and we can drop them in a future release.

This PR also includes type checking on the options and support for the __all__ value to select all fields (which mirrors DRF).

Questions

In a future I think we should enforce that either fields or exclude should be required on all DjangoObjectTypes. What you think @phalt @zbyte64 @patrick91 @mvanlonden @dopeboy ?

Also do you think we should start logging PendingDeprecationWarning for only_fields and exclude_fields?

docs/queries.rst Outdated Show resolved Hide resolved
Copy link
Collaborator

@zbyte64 zbyte64 left a comment

Choose a reason for hiding this comment

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

👍 for deprecating "only_fields", to quote the zen of python: "There should be one-- and preferably only one --obvious way to do it."

@jkimbo jkimbo force-pushed the alias-only-exclude-fields branch from dc330e7 to 730e17f Compare July 9, 2019 12:32
@jkimbo
Copy link
Member Author

jkimbo commented Jul 9, 2019

@zbyte64 I agree. I've updated the PR to start raising PendingDeprecationWarnings if only_fields and exclude_fields are used. They can be increased to DeprecationWarnings in v3.

@jkimbo jkimbo merged commit b7e4937 into master Jul 9, 2019
@jkimbo jkimbo deleted the alias-only-exclude-fields branch July 9, 2019 13:03
This was referenced Jul 9, 2019
@jtratner
Copy link

+1 to requiring that you specify either fields or exclude!

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.

None yet

6 participants