Remove dependency on Redis::Namespace

[bschaeffer]

I was just wondering how open everyone would be to a future version removing the dependency on redis-namespace.

It seems like the Split.redis= is basically just doing some magic that accepts either:

• A url (either from ENV['REDIS_URL'] or one supplied during configuration),
• a Redis instance, which is then namespaced automatically, or
• a Redis::Namespace instance.

Right now, the URL doesn't necessarily need to adhere to the spec. I would be happy to work on a solution that simplifies the assignment to allow either:

1. A string as a fully-qualified redis URL. This would still default to ENV['REDIS_URL'] and would be passed as {url: self.configuration.redis_url} to Redis.new.
2. A hash passed directly to Redis.new.
3. Otherwise assume a user supplied Redis instance. If the user wants to use Redis::Namespace, adding that dependency and supplying that connection would be up to them.

Any thoughts? I know removing the dependency would break default installations, but the upgrade path would be as simple as adding redis-namespace and supplying that to the redis assignment.

[andrew]

@bschaeffer is redis-namespace causing you problems or do you just want to remove the extra dependency? I'm open to removing it and making it an optional extra, will have to bump the version to 3.0.0 in that case, fancy sending a pr?

[bschaeffer]

It's not causing me a problem at all, just thought it was an unneeded dependency that could be left up to the user. I am definitely open to sending a PR.

My thoughts were:

• Switch the configuration to config.redis, defaulting to ENV['REDIS_URL']
• Allow either a String, a Hash, or assume a Redis connection.

Example:

def redis=(server)
  @redis = if server.is_a?(String)
    Redis.new(url: server)
  elsif server.is_a?(Hash)
    Redis.new(server)
  elsif server.respond_to?(:smembers)
    server
  else
    raise ArgumentError,
      "You must supply a url string, options hash or valid redis connection"
  end
end

We could check for any command before assigning the supplied connection. Could also eliminate the check all together and assume the user supplied a valid connection.

If that looks good I'd be happy to send a PR.

[andrew]

Yeah that looks good, pr would be awesome!

[bschaeffer]

@andrew I hesitated adding any additional documentation about removing the redis-namespace dependency, but it might be a good idea to say something about that in the docs. We could add a section specifically about how to use it.

[coveralls]

Coverage increased (+0.3%) to 98.263% when pulling aac0c10 on bschaeffer:remove-redis-namespace into 4c2dcd8 on splitrb:master.

[coveralls]

Coverage increased (+0.3%) to 98.263% when pulling aac0c10 on bschaeffer:remove-redis-namespace into 4c2dcd8 on splitrb:master.

[andrew]

@bschaeffer I think we're going to need to make this a little bit more backwards compatible, currently everyone who's using redis_url in their config is going to get an error when they upgrade, perhaps we can start with a deprecation warning?

[bschaeffer]

That sounds good. I am not used to contributing such breaking changes. Good catch and I'll be more than happy to make those changes.

[bschaeffer]

@andrew Made the change, and I'd be more than happy to rebase these commits. Up to you.

[coveralls]

Coverage increased (+0.3%) to 98.273% when pulling 8fc8414 on bschaeffer:remove-redis-namespace into 4c2dcd8 on splitrb:master.

[coveralls]

Coverage increased (+0.3%) to 98.273% when pulling 5c0b148 on bschaeffer:remove-redis-namespace into 4c2dcd8 on splitrb:master.

[coveralls]

Coverage increased (+0.3%) to 98.273% when pulling 5c0b148 on bschaeffer:remove-redis-namespace into 4c2dcd8 on splitrb:master.

[coveralls]

Coverage increased (+0.3%) to 98.273% when pulling 5c0b148 on bschaeffer:remove-redis-namespace into 4c2dcd8 on splitrb:master.

[andrew]

👍

[lime]

This seems to be a breaking change, in the sense that Split won't find my existing experiment data after upgrading, unless I manually reinstate the split namespace. Or am I reading it right?

If so, it would probably be helpful to mention this more explicitly in the changelog. As stated above, a bump to 3.0 could have been appropriate.

No harm done on my part, but I could see how this might bite someone else down the road. :)

[bschaeffer]

@lime I thought it was going to be a bump to 3.0. Workaround is to add redis-namespace to your Gemfile and pass that into split:

redis = Redis::Namespace.new(url: 'redis://localhost:6379', namespace: 'split')
Split.redis = redis

[andrew]

Sorry my mistake, should have been released as 3.0.0