Skip to content

Update #sadd to #sadd?#207

Closed
oscaredel wants to merge 3 commits intoresque:masterfrom
oscaredel:update-sadd
Closed

Update #sadd to #sadd?#207
oscaredel wants to merge 3 commits intoresque:masterfrom
oscaredel:update-sadd

Conversation

@oscaredel
Copy link
Copy Markdown

Redis now gives this deprecation warning:

Redis#sadd will always return an Integer in Redis 5.0.0. Use Redis#sadd? instead.(called from: /usr/local/bundle/gems/redis-namespace-1.8.0/lib/redis/namespace.rb:476:in `call_with_namespace')

@oscaredel
Copy link
Copy Markdown
Author

I don't really know why one of the tests is failing. So for now this PR is more of a suggested change. :)

@ojab
Copy link
Copy Markdown

ojab commented Aug 25, 2022

Looking into master branch, I guess it hangs from time to time, there are a bunch of cancelled jobs for jruby-9.2.
Probably rerunning of the failed job or repushing, if button is not available, will help.

end

it "should properly intersect three sets" do
@namespaced.sadd('foo', 1)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

try

    @namespaced.sadd('foo', [1])
    @namespaced.sadd?('foo', 1)

@@ -240,22 +240,22 @@
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

probably worth adding

Redis.raise_deprecations = true

@jwoodrow
Copy link
Copy Markdown

jwoodrow commented Sep 5, 2022

Hi 👋

Can we get an update on when this might be merged ? The deprecation message is very polluting for loggers like datadog /coralogix & such (which reflects on the monthly cost)

@oscaredel
Copy link
Copy Markdown
Author

oscaredel commented Sep 5, 2022

I'm also wondering if this will actually solve the warning, since #sadd will still be used in that call_with_namespace method? What is using that call_with_namespace method and should that call also be adjusted to actually call it with #sadd? instead of #sadd?

I don't have access to the stack-trace now, since I'm enjoying some time off. :)

Otherwise it might still show the warning like it also does for this one: #195

@bf4
Copy link
Copy Markdown

bf4 commented Sep 5, 2022

If I were a maintainer, I would not merge this as is because it removes tests for a feature still used

@oscaredel
Copy link
Copy Markdown
Author

oscaredel commented Sep 15, 2022

Since several gems now already lock our redis version to < 5.0, mainly Sidekiq, I've just ignored the remaining warnings.
So no need for this for me anymore. Feel free to iterate on it.

Also it seems like we don't get this specific warning anymore after upgrading to the latest sidekiq version.

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.

4 participants