-
Notifications
You must be signed in to change notification settings - Fork 1
Add caching for client get #79
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 caching for client get #79
Conversation
Co-authored-by: Joseph Ware <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #79 +/- ##
==========================================
+ Coverage 98.24% 98.63% +0.38%
==========================================
Files 5 5
Lines 57 73 +16
==========================================
+ Hits 56 72 +16
Misses 1 1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
… into 66-implement-caching-on-get_configuration
olliesilvester
left a comment
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! Just a few minor comments, happy to discuss if you disagree/they don't make sense
olliesilvester
left a comment
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.
Thanks, I've made some more suggestions to try and keep the logic more simple
olliesilvester
left a comment
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.
The logic + tests looks great now, thanks. I have some opinionated requests for some of the comments, then I will approve
Co-authored-by: Joseph Ware <[email protected]>
…on-get_configuration
…m:DiamondLightSource/daq-config-server into 66-implement-caching-on-get_configuration
olliesilvester
left a comment
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.
Thanks! I will wait to resolve that conversation with @DiamondJoseph before merging
Fixes #66