MiniProfiler / rack-mini-profiler

Profiler for your development and production Ruby rack apps.
MIT License
3.73k stars 406 forks source link

Remove Redis 4.2 warnings. #450

Closed tgxworld closed 4 years ago

tgxworld commented 4 years ago

Apps on Redis 4.2 and up will see a warning due to https://github.com/redis/redis-rb/commit/325752764995b02f17c3e5240ea489f641911d7d

SamSaffron commented 4 years ago

@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 commented 4 years ago

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

SamSaffron commented 4 years ago

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

tgxworld commented 4 years ago

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

casperisfine commented 4 years ago

Should we monkey patch it in and keep this code?

Yeah, based on your constraints I'd do this: https://github.com/MiniProfiler/rack-mini-profiler/commit/44dee3dc008e9addc1b029178980d1bc71be90f2

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
renchap commented 4 years ago

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 commented 4 years ago

@renchap sure, just pushed a release.

renchap commented 4 years ago

Amazing thanks!