-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Allow accessing enum value names via methods #5206
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.
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.downcase
conflicts 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: false
could 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! |
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_name
Now:
Jazz::Family.string