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

Apply camel case converter to field names in DRF errors #514

Merged
merged 2 commits into from
Jun 25, 2019

Conversation

kalekseev
Copy link
Contributor

This change eliminates inconsistency between schema field names and serializer errors field names

@coveralls
Copy link

Coverage Status

Coverage increased (+0.005%) to 94.523% when pulling adb0947 on kalekseev:camel-case-drf-errors into f76f38e on graphql-python:master.

@kalekseev
Copy link
Contributor Author

Though TypeError has type string[], I think nested serializers should be supported too? like if errors = {entity: { option_a: ["This Field is required"] }} the option_a should be converted to optionA, what do you think?

@patrick91
Copy link
Member

I quite like this, even thought I think I'll rewrite the implementation of the errors for the serializer mutation, but I'll probably keep the current implementation for backwards compatibility.

@syrusakbary what do think of this pull request? It breaks the compatibility with user using errors and having fields with _ in them, but it is definitely nicer than before I guess

@artofhuman
Copy link
Contributor

@kalekseev
Copy link
Contributor Author

Should form errors convert to camel case too? https://github.com/graphql-python/graphene-django/blob/master/graphene_django/forms/mutation.py#L49

I can implement that in this pr but both are breaking changes so need decision from author first.

@artofhuman
Copy link
Contributor

artofhuman commented Mar 21, 2019

Maybe for easy release we need another option in the schema. Like:

schema = Schema(Query, auto_camelcase=True, errors_camelcase=True)

Or if it related only for this packed use some global config like DJANGO_GRAPHENE_CAMELCASE_ERRORS=True

And check this options here with deprecation message. In major release check only auto_camelcase

And what if auto_camelcase=False, i think we should not use camelcase for errors. WDYT?

@stale
Copy link

stale bot commented Jun 11, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jun 11, 2019
@jkimbo
Copy link
Member

jkimbo commented Jun 11, 2019

This looks like it's still a useful change so don't close this stalebot!

@kalekseev so that we can get this merged without it being a breaking change could you add a flag like DJANGO_GRAPHENE_CAMELCASE_ERRORS=True that @artofhuman suggested?

@stale stale bot removed the wontfix label Jun 11, 2019
@kalekseev kalekseev force-pushed the camel-case-drf-errors branch from adb0947 to e55363e Compare June 12, 2019 19:27
@kalekseev kalekseev force-pushed the camel-case-drf-errors branch from e55363e to d2b4a02 Compare June 12, 2019 19:31
@kalekseev
Copy link
Contributor Author

Hi guys, updated pr with recursive conversion to camel case for nested serializers support, added flag to enable this change.

@artofhuman
Copy link
Contributor

👍

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.

Love the ErrorType.from_errors method.

graphene_django/utils/utils.py Show resolved Hide resolved
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.

This looks great thanks @kalekseev !

@jkimbo jkimbo merged commit e2e496f into graphql-python:master Jun 25, 2019
@jkimbo jkimbo mentioned this pull request Jul 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants