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

Code & comment for redis initializer method could be clearer #1898

Open
matthewhively opened this issue Mar 14, 2024 · 0 comments
Open

Code & comment for redis initializer method could be clearer #1898

matthewhively opened this issue Mar 14, 2024 · 0 comments

Comments

@matthewhively
Copy link
Contributor

resque/lib/resque.rb

Lines 106 to 136 in 2f9d080

# Accepts:
# 1. A 'hostname:port' String
# 2. A 'hostname:port:db' String (to select the Redis db)
# 3. A 'hostname:port/namespace' String (to set the Redis namespace)
# 4. A Redis URL String 'redis://host:port'
# 5. An instance of `Redis`, `Redis::Client`, `Redis::DistRedis`,
# or `Redis::Namespace`.
# 6. An Hash of a redis connection {:host => 'localhost', :port => 6379, :db => 0}
def redis=(server)
case server
when String
if server =~ /rediss?\:\/\//
redis = Redis.new(:url => server)
else
server, namespace = server.split('/', 2)
host, port, db = server.split(':')
redis = Redis.new(:host => host, :port => port, :db => db)
end
namespace ||= :resque
@data_store = Resque::DataStore.new(Redis::Namespace.new(namespace, :redis => redis))
when Redis::Namespace
@data_store = Resque::DataStore.new(server)
when Resque::DataStore
@data_store = server
when Hash
@data_store = Resque::DataStore.new(Redis::Namespace.new(:resque, :redis => Redis.new(server)))
else
@data_store = Resque::DataStore.new(Redis::Namespace.new(:resque, :redis => server))
end
end

I was looking through the initializer for the Resque.redis connection and thought of a few things.

  1. Based on my understanding of the initializer, I believe the comment above this method should be reworded as follows:
# Accepts: 
#   1. A String in any of the following forms
#       a. 'hostname:port'
#       b. 'hostname:port:db'
#       c. 'hostname:port:db/namespace'
#       d. 'hostname:port/namespace'
#       e. 'redis://' or 'rediss://' + 'hostname:port'
#   2. Instances of classes:
#       `Redis`
#       `Redis::Client`
#       `Redis::DistRedis`
#       `Redis::Namespace`
#       `Resque::DataStore`
#   3. A redis connection Hash  {:host => 'localhost', :port => 6379, :db => 0} 
  1. When initializing a regular Redis server, it accepts a string in this form
    <hostname>:<port>/<db>, no concept of namespace obviously
    This could lead to confusion since Resque.redis accepts a string in this form
    <hostname>:<port>:<db>/<namespace>
    It may be worth a warning in here somewhere of that subtle difference in string definition. Technically this could effect initialization via point 1e above.
  2. The line if server =~ /rediss?\:\/\// could be replaced with if server.start_with?(%r{rediss?://}) for ease of reading (less regex escaping)
  3. It seems odd that the most consistent way to set the namespace is outlined in the README as Resque.redis.namespace = 'foobar'. One would think with the stringified ways of choosing the namespace at init time, there would be a way to do that for initializers other than STRING or Redis::Namespace or Resque::DataStore objects. I don't have a better way to suggest though.
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

No branches or pull requests

1 participant