-
Notifications
You must be signed in to change notification settings - Fork 375
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 "multi" methods instrumentation for Rails cache integration #1217
Conversation
746d934
to
b546cc9
Compare
👋 @michaelkl thanks for the contribution! Generally this looks like a really solid addition to the library. I am a bit backlogged right now but should be able to give this a thoughtful review in the coming days, I want to review how some other libraries and languages handle Also, for the linting failed tests, |
b546cc9
to
4516f85
Compare
Thank you @ericmustin ! Glad to help the community! I will remove the Draft status from the PR when it turns out ready to be reviewed. Take your time, no rush at all! |
d8403dd
to
84ebc85
Compare
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.
Great work @michaelkl! I left a few suggestion on my review, but it looks very close to completion. Great job on getting it to pass tests on all (even very old) versions of Rails 👍
84ebc85
to
0a08188
Compare
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.
Thank you very much, @michaelkl! 🚀
In current implementation only
read
,write
andfetch
methods ofActiveSupport::Cache::Store
are instrumented. This PR adds the instrumentation ofread_multi
,write_multi
andfetch_multi
methods.