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

Warn if fields or exclude are not defined on DjangoObjectType #981

Merged
merged 5 commits into from Jun 11, 2020

Conversation

radekwlsk
Copy link
Contributor

@radekwlsk radekwlsk commented Jun 7, 2020

To make use of Django-Graphene more familiar to folks using DRF PendingDeprecationWarning DeprecationWarning (edit: changed in 805480) is issued when DjangoObjectType has neither fields nor exclude option set.

Current deprecations of exclude_fields and only_fields are taken into account as this check happens after they are copied over to fields or exclude.

All tests, examples and docs are also updated so that they won't issue this warning. Documentation mentions that warning in docs/queries.rst#specifying-which-fields-to-include and instructs how to avoid it.

Change is part of v3 release discussed in #705.

Closes #710

@radekwlsk radekwlsk force-pushed the topic-710 branch 2 times, most recently from 9d7fb3b to d318119 Compare June 7, 2020 10:51
@radekwlsk radekwlsk changed the base branch from master to v3 June 7, 2020 11:45
@radekwlsk
Copy link
Contributor Author

@jkimbo let me know if the tests and docs changes are expected here or if I should just leave it throwing warnings and don't mention in docs until it is DeprecationWarning instead of PendingDeprecationWarning.

@jkimbo
Copy link
Member

jkimbo commented Jun 10, 2020

@radekwlsk thanks, this is great. Mentioning it in the docs is great too.

Thinking about it though maybe we should raise DeprecationWarning instead of PendingDeprecationWarning for the v3 release. What do you think?

@jkimbo jkimbo mentioned this pull request Jun 10, 2020
7 tasks
@radekwlsk
Copy link
Contributor Author

@jkimbo sure, with major release it would be too bad. Especially if it is mentioned in the v2 to v3 migration notes.

I will change it tomorrow then.

* as it is a major release 3, start right away from DeprecationWarning for ObjectTypes missing fields or exclude option
* updated docs information to be more forbidding
@radekwlsk
Copy link
Contributor Author

@jkimbo Warning level bumped to DeprecationWarning and I've also updated the information in docs to be have stronger tone about this soft requirement.

Copy link
Member

@jkimbo jkimbo left a comment

Choose a reason for hiding this comment

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

Thanks @radekwlsk !

@jkimbo jkimbo merged commit 1f752b6 into graphql-python:v3 Jun 11, 2020
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.

Start warning if fields or exclude are not defined on DjangoObjectType
2 participants