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

Add support for redis-rb >= 4.3 #347

Merged
merged 1 commit into from
Aug 14, 2021
Merged

Add support for redis-rb >= 4.3 #347

merged 1 commit into from
Aug 14, 2021

Conversation

marcelolx
Copy link
Member

Follow-up of #346

When that PR gets merged we need to add support for redis-rb >= 4.3 which will return an integer value for exists while prior versions return a boolean value.

Fixing the warning described in original PR #335 will be made in a future PR and will be included only in sidekiq-scheduler v4 as described here #345 (comment)

@marcelolx marcelolx requested a review from juan-apa July 17, 2021 23:37
Comment on lines 63 to 68
case r.exists(key)
when true, 1
true
else
!!r.exists(key)
false
end
Copy link

Choose a reason for hiding this comment

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

1 and true evaluate to a truthy value, so doing r.exists(key) ? true : false would do the trick here

Copy link
Member Author

Choose a reason for hiding this comment

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

@juan-apa Yeah, but 0 also will evaluate to a truthy value

irb(main):001:0> true ? 'A' : 'B'
=> "A"
irb(main):002:0> 1 ? 'A' : 'B'
=> "A"
irb(main):003:0> 0 ? 'A' : 'B'
=> "A"
irb(main):004:0> false ? 'A' : 'B'
=> "B"

Choose a reason for hiding this comment

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

oh wow. I mean, makes sense, but at the same time it doesn't. It's fine by me if you keep it with the case then.

@marcelolx marcelolx merged commit e510cd2 into master Aug 14, 2021
@marcelolx marcelolx deleted the support-redis-exists branch August 14, 2021 17:43
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.

None yet

2 participants