-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Allow accessing enum value names via methods #5206
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
Conversation
rmosolgo
left a comment
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.
Hey, thanks so much for this contribution and sorry I didn't get back to you til now. I'm definitely open to this change. I think the next few things to work out are:
-
What if the value's
graphql_name.downcaseconflicts with an existing method onSchema::Enum, likevalue,kind, orinherited? I think we should emit some kind of warning that the method generation was skipped and provide an override option likevalue_method:, whose value will be used instead ofgraphql_name.downcase. -
In case this causes some problems for people who are already using the gem, I'd like to have some way for them to turn it off. For example, if a
value_method:option was added,value_method: falsecould disable creating these methods, and they could set that option by default in their base enum value classes.
Thanks, and let me know what you think!
f9d596b to
21dc28a
Compare
|
Thanks for the feedback, I did my best to address everything! I also added a CI fix for unavailable Logger class |
lib/graphql/schema/enum.rb
Outdated
| value_method_name = configured_value_method || value.graphql_name.downcase | ||
|
|
||
| if respond_to?(value_method_name.to_sym) | ||
| $stderr << "Failed to define value method for :#{value_method_name}, because " \ |
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.
Out of curiousity ... is this different than warn(...) ?
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.
nope :)
|
Thanks for this improvement! |
For future consideration, this should have been an opt-in rather than an opt-out, IMHO. Anything that adds strictly-convenience APIs with unlimited name cardinality to an existing schema structure should be done with care. This is particularly aggressive as a patch version change. |
|
Hey @gmac, sorry for the trouble with this. IMO it's not too late to make it opt-in. Could you describe the trouble you encountered? |
|
We have a lot of superclassing over all major GraphQL structures to provide various internal concerns. It’s always a measured risk adding atop base library APIs, but they tend to evolve slowly and in isolated ways when they change in a conflicting manner. This change effectively created N new potentially conflicting APIs, where N is our own enum value surface area. I just overrode the enum constructor to disable the new methods everywhere, but it feels like that should have been the default. |
|
I've opened this as a formal bug in #5254. This really should be off-by-default. |
This is a pretty minor change, but might be helpful: now you can grab GraphQL name of the enum value using the method instead of a complex spell.
Before:
Jazz::Family.values["STRING"].graphql_nameNow:
Jazz::Family.string