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

Fix arity bug in respond_to? method #3

Merged
merged 1 commit into from
Oct 11, 2017
Merged

Fix arity bug in respond_to? method #3

merged 1 commit into from
Oct 11, 2017

Conversation

ZJvandeWeg
Copy link
Contributor

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 02c54fe on ZJvandeWeg:zj-fix-arity-bug into 5f940fb on exAspArk:master.

@ZJvandeWeg
Copy link
Contributor Author

@exAspArk It seemed not really needed to add a test. Please let me know what else you might need before merging/

@exAspArk
Copy link
Owner

exAspArk commented Oct 11, 2017

@ZJvandeWeg looks great! Thank you! 🙌

Since you dig into the source code, may I ask you a question: How do you use the gem? With GraphQL, RESTful API or anything else?

In the version 1.x, I decided to try the "magical" automatic syncing which potentially makes it easier to use BatchLoader without GraphQL:

When we call any method on the lazy object, it'll be automatically loaded

Because of this, the source code looks a bit complicated (compared to the code in 0.x branch) and leads to bugs like this one. I'm thinking whether the automatic loading worth it in general or not. Will be happy to hear your opinion :)

@exAspArk exAspArk merged commit a66165e into exAspArk:master Oct 11, 2017
@ZJvandeWeg ZJvandeWeg deleted the zj-fix-arity-bug branch October 11, 2017 19:37
@ZJvandeWeg
Copy link
Contributor Author

@exAspArk I'm investigating if we, GitLab, can use it for another datasource. Our backend for git is called Gitaly, and like with databases, it doesn't have a magic solution for n + 1 queries on it.

On the magic loading, I kinda like it, but it was confusing at first to get started with it. And like you said, the code needed a few times reading and byebugging through it before I fully understood what was going on and why. I also worry a bit about the adoption, given the lazy loading has a few gotchas and the API.

Other than that, I'll update you in a week or so. 😄

@exAspArk
Copy link
Owner

exAspArk commented Oct 12, 2017

@ZJvandeWeg thanks a lot for the feedback! Looking forward to hearing from you 😸

P.S I released the fix in the 1.0.4 version

@ZJvandeWeg
Copy link
Contributor Author

@exAspArk Sorry, I forgot to give extra feedback. We've now merged the first merge request using batch-loader: specifically on https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/15370/diffs#f34dbf3de4996b6652de2cbafe00a9a79e516365_79_79

If you have any feedback, please let me know. I've one minor question, on which I'll open an issue.

@exAspArk
Copy link
Owner

@ZJvandeWeg wow, thank you! That's awesome! 💥

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.

3 participants