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

Allow accessing enum value names via methods #5206

Merged
merged 8 commits into from
Jan 28, 2025

Conversation

DmitryTsepelev
Copy link
Contributor

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

@DmitryTsepelev DmitryTsepelev marked this pull request as ready for review January 12, 2025 17:24
Copy link
Owner

@rmosolgo rmosolgo left a 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 on Schema::Enum, like value, kind, or inherited? I think we should emit some kind of warning that the method generation was skipped and provide an override option like value_method:, whose value will be used instead of graphql_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!

guides/type_definitions/enums.md Outdated Show resolved Hide resolved
lib/graphql/schema/enum.rb Outdated Show resolved Hide resolved
@DmitryTsepelev DmitryTsepelev marked this pull request as draft January 25, 2025 12:38
@DmitryTsepelev
Copy link
Contributor Author

Thanks for the feedback, I did my best to address everything! I also added a CI fix for unavailable Logger class

@DmitryTsepelev DmitryTsepelev marked this pull request as ready for review January 25, 2025 16:16
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 " \
Copy link
Owner

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(...) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope :)

@rmosolgo rmosolgo merged commit 5bc1614 into rmosolgo:master Jan 28, 2025
14 of 15 checks passed
@rmosolgo
Copy link
Owner

Thanks for this improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants