Symbolize all the option keys at Redis::Client#initialize(option)

[aeroastro]

Symbolize all the option keys at Redis::Client#initialize(option)

Previously, a limited portion of options are symbolized at
Redis::Client#initialize(option). Specifically, only those defined at
Redis::Client::DEFAULTS are symbolized.

This invoked confusing behavior and increased maintenance cost.
For example, specifying timeout was a little bit tricky as follows.

Redis::Client.new('timeout' => 10.0).options[:timeout]
# => 10.0
Redis::Client.new('read_timeout' => 10.0).options[:read_timeout]
# => 5.0

Now, all the keys of options are symbolized, and consistent
external behavior is achieved.

Redis::Client.new('timeout' => 10.0).options[:timeout]
# => 10.0
Redis::Client.new('read_timeout' => 10.0).options[:read_timeout]
# => 10.0

This can also reduce maintenance cost of Redis::Client::DEFAULTS.

---

This idea was rejected because of memory leak at #256 in 2012.

However, in 2020, I believe we can safely symbolize keys because of the following reasons.

• Rational developers won't pass hashes containing a large number of unsupported keys to external libraries as their options, especially on environments where memory leak is a problem.
• In most cases, unintentionally inserted keys in options will not change dynamically until the program ends, which means unintentionally used memory size by converted symbols is limited (fixed).
• Even if some developers pass the options containing a large number of dynamically-changing unsupported keys, currently (November, 2020) supported ruby versions in gemspec, which is 2.3 and later, have Symbol GC functionality and memory leak is restrained to some extent.

Also, nobody has been interested in the previous style Pull Request at #689, which I created 3 years ago.
I assume that it is about time to change our strategy to symbolize keys.

[byroot]

If anything, I'd much rather go in the direction of making all these options keyword arguments of the constructor.

The large number of different and inconsistent ways to instantiate a Redis client have created countless problems over the years. I'd prefer to make it stricter rather than laxer.

[byroot]

I'm curious, what is even the use case for string keys options?

[aeroastro]

Thank you for your prompt response ⭐

I'd prefer to make it stricter rather than laxer.

What I would like to achieve is the consistency, so stricter direction is O.K. for me.
Then, how about #689?
Or, are we going to deprecate the use of string key as an option?

I'm curious, what is even the use case for string keys options?

Actually, I had a trouble due to the current inconsistent behavior as follows.

Let's assume we have the following code somewhere in the application.

::Redis::Client.new(YAML.load_file('redis.yml'))

In this case, we can alter the settings just by editing redis.yml, and the code works as long as the developer use only the keys defined at Redis::Client::DEFAULTS.

However, following change unintentionally breaks timeout settings.
Contrary to a developer's intention to decrease timeout value, actual timeout value increases to 5.0 sec.

host: localhost
port: 6379
db: 10
- timeout: 1.0
+ connect_timeout: 1.0
+ read_timeout: 0.3
+ write_timeout: 0.3

I just would like to fix this inconsistency and unintentional behavior.

If string keys are properly symbolized, all the setting work.
If string keys are ignored or regarded as ArgumentError, a developer can fix the code at the very beginning.

[byroot]

Then, how about #689?
Or, are we going to deprecate the use of string key as an option?

I'd like to deprecate string keys, and remove them for redis-rb 5 yes. In the meantime it would make sense to merge #689.

I just would like to fix this inconsistency and unintentional behavior.

Ok, so no real use case for string keys. Just for consistency. I get it.

If you don't mind I'll close this, let's revive #689. Can you rebase it and ping me once it's done?