-
Notifications
You must be signed in to change notification settings - Fork 768
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
fix typed choices, make working with different Django 5x choices options #1539
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR. You mention that this should fix #1476. Could you please add a test or two accordingly for the mutation behavior, to ensure it's fixed and won't regress in the future?
graphene_django/converter.py
Outdated
@@ -61,6 +60,24 @@ def wrapped_resolver(*args, **kwargs): | |||
return blank_field_wrapper(resolver) | |||
|
|||
|
|||
class EnumValueField(Field): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this effectively replaces BlankValueField
. Can you add a comment or something stating as much, and "marking" BlankValueField
as deprecated? I assume it's effectively only ever been used internally, but we shouldn't remove it without a major version bump, unfortunately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, my idea was to inherit from the BlankValueField
class, but I forgot to. The interesting thing is that I've seen the test that was added when this class was added (test_choice_enum_blank_value
), but it didn't catch the bug in my code as the test itself was wrong (the field in the test instance was null instead of blank).
I've tested a bug in the test and classes inheritance, so they match my initial idea, but if you want me to move all the logic to the EnumValueField
and deprecate BlankValueField
, I can do that
34ed25e
to
176b3e2
Compare
the source of the problem was that the library incorrectly resolved typed choice values assigned to model choice fields (which is a totally valid case and can happen not only in mutations). I've added a |
Would you be able to add a test for the mutation specifically, for comprehensiveness and good measure? |
176b3e2
to
ff99248
Compare
extended the added test with the mutation call. Thanks for the suggestion! |
An attempt to fix #1476
While working on the fix, I realized that Graphene doesn't support choice fields initialized with a types class itself without calling
choices
. Fixed this as well.Django 5.1 introduced a convenient
normalize_choices
method that I use if a newer django version installed