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

Verified

This commit was signed with the committer’s verified signature.
drazisil-codecov Joe Becher
@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!

@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!

@gmac
Copy link
Contributor

gmac commented Feb 12, 2025

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.

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.

@rmosolgo
Copy link
Owner

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?

@gmac
Copy link
Contributor

gmac commented Feb 17, 2025

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.

@gmac
Copy link
Contributor

gmac commented Feb 26, 2025

I've opened this as a formal bug in #5254. This really should be off-by-default.

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.

None yet

3 participants