Skip to content

Correctly resolve Enum #879

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

Closed
wants to merge 6 commits into from
Closed

Correctly resolve Enum #879

wants to merge 6 commits into from

Conversation

avivey
Copy link

@avivey avivey commented Dec 17, 2018

See #877, #756, #444.

This changes introduces "new style enums" which are more natural to work with, having the value provided to the resolver is an enum value.
This only works with Python-Enum based enums (graphene.Enum.from_enum()) and is disabled by default (pass legacy_enum_resolver=False to enable).

There's still a bug with specifying default values to enum arguments, but that bug is in graphql-core, and there's no reasonable workaround in graphene for it. We'll need graphql-core-next to better handle enums.

In part, this change reverts #446 for new-style enums. #446 added a custom __eq__ to the underlying python-enum backing graphene-enums. Some magic in the Python stack expects that if you provide __eq__ you should also provide __hash__ (Which makes sense), rendering the resulting enum instances un-hashable.
Since having EnumInstance == EnumInstance.Value doesn't make sense in a typed world (such as GraphQL), I prefer to remove this and resolve the underlying problem instead (provide resolvers with proper values).

@avivey
Copy link
Author

avivey commented Dec 17, 2018

@ekampf

@ekampf
Copy link
Contributor

ekampf commented Jan 4, 2019

This is a breaking change.
So I guess this would require a version major\minor change...

@syrusakbary whats your policy on such changes?

@ekampf
Copy link
Contributor

ekampf commented Mar 9, 2019

@avivey let's discuss how we can make this backward-compatible.
It should work the old way by default until we make the cut to Python3...

@ekampf ekampf requested review from ekampf and ProjectCheshire March 9, 2019 01:11
@allardhoeve
Copy link

What is needed for this to enter master? What is breaking about the change?

@ekampf
Copy link
Contributor

ekampf commented Apr 8, 2019

@allardhoeve the breaking change is that when . you have Enum arguments you'll now get the enum and not its value.
Say your enum is

class Episode(graphene.Enum):
        NEWHOPE = 4
        EMPIRE = 5
        JEDI = 6

What you're getting in graphene now wuld be Episode.NEWHOPE.value and what you'll get after is Episode.NEWHOPE

We'll work to figure out how to make this backward compatible (have a flag somewhere indicating new\old enum support)

@avivey
Copy link
Author

avivey commented Apr 13, 2019

@ekampf now supports old form by default.
New form only supported for the from_enum() form, because hopefully some day we can drop the class Xx(Enum): form and remove a lot of code from that.

@phalt
Copy link

phalt commented May 7, 2019

@dschaller @avivey we could bundle this over the graphene3 release?

@ProjectCheshire ProjectCheshire added 3.0 Fix/Release version 3.0 and removed scheduled_for_v3 labels Jun 2, 2019
@stale
Copy link

stale bot commented Jul 29, 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 Jul 29, 2019
@jkimbo jkimbo removed the wontfix label Jul 30, 2019
@pngnviko
Copy link

pngnviko commented Aug 5, 2019

ping

@stale
Copy link

stale bot commented Oct 4, 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 Oct 4, 2019
@stale stale bot closed this Oct 11, 2019
@ekampf ekampf removed the wontfix label Feb 14, 2020
@avivey
Copy link
Author

avivey commented Feb 15, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.0 Fix/Release version 3.0 work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants