-
Notifications
You must be signed in to change notification settings - Fork 23
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
Improved storage capabilities #63
base: master
Are you sure you want to change the base?
Conversation
Would you still like to merge this? I'm happy to review if so. |
Hey, for now i depend on my own fork and think others would, if they want to, profit from the extended storage capabilities as well. |
Amazing, thanks! Let me set myself as reviewer and add a few comments :) |
@@ -4,12 +4,18 @@ | |||
|
|||
|
|||
class Storage: |
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.
How about making this class abstract to ensure the methods are implemented?
class Storage: | |
class Storage(ABC): |
def __init__(self): | ||
self.redis = None | ||
|
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.
I don't understand why the base Storage
class needs to have the redis
attribute defined. If anything, for consistency with the InMemoryStorage
, it should be renamed to _storage
result_str = result_bytes.decode("utf-8") | ||
result_dict = json.loads(result_str) | ||
return result_dict | ||
except Exception as e: | ||
raise StorageError(f"Redis load failed: {e}") | ||
|
||
def delete(self, *keys: str) -> int: |
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.
Totally optional but how do you feel about adding some unit testing for the new function?
because the current implementation of
storage.read()
orstorage.save()
do not work well when they run simultaneously i moved the_redis
from storage class toredis
.now the entire redis class is available within the storage class.
furthermore
storage.delete()
is now a thing.i have a command that fetches content from a website. this takes time.
to make sure a user never receives the same post twice i used to:
if the user runs the same command twice in a row it might happen that both coroutines read the same data and only the last one is actually saved.
now this is possible:
p.s.: sorry to spam your repo. i messed up my git settings... new pc and stuff...