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

Remove Redis 4.2 warnings. #450

Merged
merged 1 commit into from Jul 14, 2020
Merged

Conversation

tgxworld
Copy link
Contributor

@tgxworld tgxworld commented Jun 16, 2020

Apps on Redis 4.2 and up will see a warning due to redis/redis-rb@3257527

@SamSaffron
Copy link
Member

@casperisfine what should our approach be here.

Our dependency on redis is "weak" in that there is no explicit dependency. exists? may no exist in earlier redis gems.

Should we monkey patch it in and keep this code?

@tgxworld
Copy link
Contributor Author

We can just implement exists? since it isn't hard. Essentially just calling @client.call([:exists, key])

@SamSaffron
Copy link
Member

as long as it is backwards compatible with reasonable redis gem releases for whatever definition of reasonable that we can come up with

@tgxworld
Copy link
Contributor Author

We're only using a single key so it should be backwards compatible all the way

@casperisfine
Copy link
Contributor

Should we monkey patch it in and keep this code?

Yeah, based on your constraints I'd do this: 44dee3d

If the redis_store.rb file was autoloaded, you could go with a slightly cleaner alternative:

unless Redis.method_defined?(:exists?)
  module RedisExistsRefinement
    refine Redis do
      def exists?(key)
        exists(key)
      end
    end
  end
  using RedisExistsRefinement
end

@tgxworld tgxworld changed the title Upgrade Redis to 4.2.1. Remove Redis 4.2 warnings. Jul 14, 2020
@SamSaffron SamSaffron merged commit 7ad4279 into MiniProfiler:master Jul 14, 2020
@tgxworld tgxworld deleted the upgrade_redis branch July 14, 2020 05:00
@renchap
Copy link

renchap commented Jul 28, 2020

Would it be possible to have a release that includes this PR? This is currently preventing our upgrade to the Redis 4.2
Thanks!

@SamSaffron
Copy link
Member

@renchap sure, just pushed a release.

@renchap
Copy link

renchap commented Jul 30, 2020

Amazing thanks!

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

4 participants