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

feat: add memcached driver #92

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

feat: add memcached driver #92

wants to merge 9 commits into from

Conversation

Sanicboi
Copy link

Added memcached support. Had to change some functions and types due to the fact that they are not supported by memcached. For instance, memcached does not support getKeys.

@Hebilicious Hebilicious added the driver label Jun 30, 2023 — with Volta.net
@Hebilicious Hebilicious self-assigned this Jun 30, 2023
@Hebilicious Hebilicious self-requested a review June 30, 2023 17:42
@Hebilicious Hebilicious requested a review from pi0 July 2, 2023 21:07
Copy link
Member

@Hebilicious Hebilicious left a comment

Choose a reason for hiding this comment

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

LGTM

@Hebilicious
Copy link
Member

Hebilicious commented Jul 2, 2023

@Sanicboi Thanks for the PR !

@pi0 Memcached does not supports getKeys. I tried implementing with cachedump, but it wasn't reliable. We could manually implement a custom solution to keep track of all the keys set with setItem (underneath a custom key).
But that would deserve a separated PR as its not as straightforward as it sounds.

If there's demand for it, we can definitely do it in a later. I've implemented a way to skip the getKeys test for one driver, but we could disable them all for memcached as most of them use getKeys underneath.

Also, the tests needs memcached to run, so I added that to the ci.

The docs clearly reflects that getKeys doesn't work with memcached, so I think it's fine.

@codecov
Copy link

codecov bot commented Jul 2, 2023

Codecov Report

Merging #92 (756d3c3) into main (0e6e023) will decrease coverage by 1.12%.
Report is 1 commits behind head on main.
The diff coverage is 97.61%.

❗ Current head 756d3c3 differs from pull request most recent head 66c856a. Consider uploading reports for the commit 66c856a to get more accurate results

@@            Coverage Diff             @@
##             main      #92      +/-   ##
==========================================
- Coverage   76.94%   75.82%   -1.12%     
==========================================
  Files          26       25       -1     
  Lines        3223     2809     -414     
  Branches      473      426      -47     
==========================================
- Hits         2480     2130     -350     
+ Misses        742      678      -64     
  Partials        1        1              
Files Changed Coverage Δ
src/drivers/memcached.ts 97.61% <97.61%> (ø)

... and 8 files with indirect coverage changes

@pi0 pi0 changed the title Memcached support feat: add memcached driver Aug 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants