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

Add support for class level prepare methods on arguments #4717

Merged
merged 3 commits into from
Dec 7, 2023

Conversation

DougEdey
Copy link
Contributor

Issue: #4715

I think this is what @eapache-opslevel was aluding to, it checks to see if the prepare method exists on the object or the owner (class method), and handles each approproiately

  • For Object, don't send the context, it'll be there already
  • For the Owner, send the context (like we do already for prepare blocks)
  • If neither exist, give a clear error instead of a no method found error.

Copy link
Contributor

@eapache-opslevel eapache-opslevel left a comment

Choose a reason for hiding this comment

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

This makes sense based on my understanding here. Left a few questions about the tests though.

spec/support/jazz.rb Outdated Show resolved Hide resolved
spec/support/jazz.rb Outdated Show resolved Hide resolved
@rmosolgo
Copy link
Owner

rmosolgo commented Dec 4, 2023

Hey, thanks for exploring this improvement!

I'm open to the proposed change, but I want to double-check my impression: could this cause problems for existing prepare: usages? I mean, if the configured prepare: method exists on a field's owner type (eg, a Field instance or an InputObject class), then the code will start choosing that method instead of the method on obj. Does that sound right to you?

If so, could the order of these conditions be reversed to avoid that possibility?

@DougEdey
Copy link
Contributor Author

DougEdey commented Dec 4, 2023

Good point @rmosolgo

If so, could the order of these conditions be reversed to avoid that possibility?

Yup, I've swapped the check order so we keep existing behaviour first!

@rmosolgo
Copy link
Owner

rmosolgo commented Dec 7, 2023

Thanks for this improvement!

@rmosolgo rmosolgo merged commit 05d120d into rmosolgo:master Dec 7, 2023
12 checks passed
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