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

provisional try to make minqlx independent from redis v2 #115

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mgaertne
Copy link
Contributor

This is a first try to make minqlx redis less reliant on redis v2 being installed. I tried to incorporate backward and forward compatibility as much as possible. I did not yet try this out, though. Basic information point were the information from redis v3 page: https://pypi.org/project/redis/3.0.0/ I ignored SETEX, LREM, TTL, and PTTL changes. Not sure whether any plugin uses these features.

This is a first try to make minqlx redis less reliant on redis v2 being installed. I tried to incorporate backward and forward compatibility as much as possible. I did not yet try this out, though. Basic information point were the information from redis v3 page: https://pypi.org/project/redis/3.0.0/ I ignored SETEX, LREM, TTL, and PTTL changes. Not sure whether any plugin uses these features.
@em92
Copy link
Collaborator

em92 commented Jan 28, 2023

From discord:

[15:48] ShiN0: the order of arguments in zincrby has been changed, instead of value, amount it's amount, value in redis >=3 (or the other way around). I checked for the second parameter filled in whether it's an int or float, and determined then whether it's a v2 call or a v3 call. Unfortunately if you call that function with a steam_id (it's an int) as value and 1 as amount, that logic might end up doing the wrong thing.

In this case, I suggest to make database class modifications, only if redis version is 3 or greater. For example:

class Redis(AbstractDatabase):
   # ...
   if redis >= (3, 0, 0):
     def zincrby(self, name, value, amount=1):
       return self.r.zincrby(name, amount, value)

     #def mset(blablabla):

In that case Database's API will be like redis-py v2's API.

@mgaertne
Copy link
Contributor Author

I think it's more complicated than that. In my plugins, I incorporated some code that checks the redis version as well, and tries to call the functions in question in the right manner. That logic sort of collides with anything put into database.py.

@em92
Copy link
Collaborator

em92 commented Jan 29, 2023

In my plugins, I incorporated some code that checks the redis version as well, and tries to call the functions in question in the right manner.

Since you switched back to redis-py v2, maybe you will revert commits with redis version checks?

@mgaertne
Copy link
Contributor Author

That's a possibility. However, I grow concerned when the redis-py documentation and minqlx's adaptation go too far apart from each other. Ideally, one could check whether value and amount are both numeric, and if so, pick the smaller of the two to be the amount in the resulting call. I'm not sure whether that heuristic would be good-enough.

added lrem and setex backwards compatability, and reworked zincrby to default on redis v2 behavior. If you want v3++ behavior, make sure to pass in value as str for all these functions.
Comment on lines 370 to 380
def setex(self, name, value_or_time, time_or_value):
if not isinstance(value_or_time, (int, timedelta)):
value = value_or_time
time = time_or_value
else:
value = time_or_value
time = value_or_time

if redis.VERSION < (3, 0):
return self.r.setex(name, time, value) # pylint: disable=W1114
return self.r.setex(name, value, time) # pylint: disable=W1114
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

2 participants