-
Notifications
You must be signed in to change notification settings - Fork 41
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
Refactor Redis Storage and add JSON support #78
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #78 +/- ##
==========================================
+ Coverage 80.55% 81.07% +0.51%
==========================================
Files 17 18 +1
Lines 967 1189 +222
==========================================
+ Hits 779 964 +185
- Misses 188 225 +37 ☔ View full report in Codecov by Sentry. |
@Spartee good to go from my end. |
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.
Nearly there. Looks good to me. Running tests now.
@@ -371,61 +486,94 @@ def delete(self, drop: bool = True): | |||
self._redis_conn.ft(self._name).dropindex(delete_documents=drop) # type: ignore | |||
|
|||
@check_connected("_redis_conn") | |||
@check_modules_present("_redis_conn") |
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.
Can we use lru_cache
to cache the calls to this within an invocated client?
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.
hmm i've not done that. are you saying cache the module list client side? I was going to recommend some kind of private attribute on the class like _modules
that gets populated first time you run it. Then next time the check is called, it looks at this attribute first, and then executes the redis command. Something like that. Lmk what you're thinking? Feels like a later optimization to me
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.
LGTM
PR introduces a new
BaseStorage
class, subclassed byHashStorage
andJsonStorage
, to handle the underlying data structures (as well as I/O) for Redis. Includes:key_separator
param for better use-customizability when constructing redis keys