search

assaf / vanity

Experiment Driven Development for Ruby
http://vanity.labnotes.org
MIT License
1.55k stars 269 forks source link

[Reverted] Use Redis#exists? and fallback to #exists which could be either boolean or integer in recent redis gem versions #382

Closed bsedin closed 2 years ago

bsedin commented 2 years ago

Hello!

First of all thanks for this amazing library!

I'm trying to upgrade redis from 4.4 to 4.6 in my project. It's rather painless but I noticed Vanity wasn't working properly as it assumes exists to return true or false which in recent redis-rb versions was changed and now it returns number of matching keys unless Redis.exists_returns_integer is set to false. But that option is deprecated and will be removed in redis-rb 5.0.

So for boolean checks it is recommended to use exists? method and right now it's a bit messy with all that possibilities out there. So I tried not to break anything by checking for exists? method which we now for sure gives us boolean result. And if that method is absent, we're probing exists and if it's numeric we are calculating > 0 if not we're assuming it's boolean.

So what do you think about that?

bensheldon commented 2 years ago

@bsedin thanks for flagging this and proposing a fix. I think the redis-namespace gem needs to be updated too to get the tests to pass: https://github.com/resque/redis-namespace/pull/171

I'm thinking we should add a Github Action matrix option to test both versions of Redis (we likely don't need to test every combo, just one with the new and old Redis).

bensheldon commented 2 years ago

Whoops, didn't realize pushing that would merge it into master 🤦

bsedin commented 2 years ago

@bensheldon Hey Ben and many thanks for fast response! Sorry my code caused master branch breaking :( Could not find time this week to finish this.

I owe you this one and I'll try to redeem myself by contributing something in future!

bensheldon commented 2 years ago

@bsedin no worries. it was my bad. I tried to push up a fix to your branch and it ended up going to the master branch 🤦

I made a new PR (#385) and it's looking very good. I'll have a release out this morning. 👍