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

Added cache interface and cachelib + noop implementations #1209

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

samredai
Copy link
Contributor

Summary

This breaks out just the cache interface from #1180 and leaves out any use of the dependency injection anywhere. I've also added a NoOpCache implementation of the CacheInterface which makes it nicer to use the dependency while also keeping it optional (A NoOpCache has the same methods so no need to do a null check for the cache everywhere).

I also added the caching docs page that describes how this works and how someone can implement their own custom cache dependency.

Test Plan

Wrote some tests to get full coverage.

  • PR has an associated issue: #
  • make check passes
  • make test shows 100% unit test coverage

Deployment Plan

Deploying this has no effect because it's not used anywhere yet.

Copy link

netlify bot commented Oct 21, 2024

Deploy Preview for thriving-cassata-78ae72 canceled.

Name Link
🔨 Latest commit 3275b2a
🔍 Latest deploy log https://app.netlify.com/sites/thriving-cassata-78ae72/deploys/67377dc4bef0c600089e7b37

Copy link
Member

@agorajek agorajek left a comment

Choose a reason for hiding this comment

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

This is super clear. Love it. Let's get it in and start using.

Copy link
Contributor

@shangyian shangyian left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for adding

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