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

Accept sentinel options even with string keys #599

Merged
merged 1 commit into from May 23, 2020

Conversation

lucaong
Copy link
Contributor

@lucaong lucaong commented Mar 16, 2016

Before, the sentinel options were assumed to have symbol keys, so string keys
were ignored. This is surprising, as other Redis options can be indifferently
passed as symbols or strings. Now string keys are allowed too.

@yagilm
Copy link

yagilm commented Mar 16, 2016

+1

@lucaong
Copy link
Contributor Author

lucaong commented Mar 17, 2016

Travis specs fail on Ruby 1.8.7 due to:

Gem::InstallError: rake requires Ruby version >= 1.9.3.
An error occurred while installing rake (11.1.1), and Bundler cannot continue.
Make sure that `gem install rake -v '11.1.1'` succeeds before bundling.

This is not related to the code changes though. Tests pass locally, but I would love to see the build pass on different Ruby versions on Travis. Could this be an issue with the version of test_unit?

@badboy
Copy link
Contributor

badboy commented Mar 18, 2016

More the fact that we don't restrict the rake version and therefore the latest one is pulled which is now incompatible with Ruby 1.8.7

We will pull the plug on 1.8.7 eventually.

@lucaong
Copy link
Contributor Author

lucaong commented Mar 21, 2016

I see ;)

@lucaong
Copy link
Contributor Author

lucaong commented Mar 21, 2016

It is possible to fix this by adding, in the gemspec, something like this:

if RUBY_VERSION == "1.8.7"
  s.add_development_dependency "rake", "~> 10.0"
else
  s.add_development_dependency "rake"
end

That would make the build run. Should I do that as part of this PR, or separately, or skip it completely?

@badboy
Copy link
Contributor

badboy commented Mar 21, 2016

I don't like the conditional there. We don't depend on any newer features, so we can stick with 10.0 for all Ruby versions for now (this is only a development dependency and should not cause trouble to other gems)

@lucaong lucaong closed this Mar 21, 2016
@lucaong lucaong reopened this Mar 21, 2016
@lucaong
Copy link
Contributor Author

lucaong commented Mar 21, 2016

OK, tests are passing now. I also had to make sure a recent version of bundler is used for CI, to fix failures on jRuby head. And close/reopen PR to force Travis to rebuild.

@keeganlow
Copy link

just helped someone who got burned by this issue - seems like the concerns about rake version mentioned above should be addressed now, since #601 is now in master.

Would this PR be ready to merge if we remove the parts that deal with the rake version? Happy to submit a modified PR myself if that helps.

@alexpapworth
Copy link

Any chance we can get this PR finished off and approved?

@lucaong
Copy link
Contributor Author

lucaong commented May 22, 2020

As far as I know, this is finished (tests were passing), although I sent it 4 years ago so things might have changed?

@lucaong lucaong force-pushed the sentinel_string_option_keys branch 2 times, most recently from 37220f0 to 3b77b39 Compare May 22, 2020 14:42
Before, the sentinel options were assumed to have symbol keys, so string keys
were ignored. This is surprising, as other Redis options can be indifferently
passed as symbols or strings. Now string keys are allowed too.
@lucaong lucaong force-pushed the sentinel_string_option_keys branch from 3b77b39 to c452de8 Compare May 22, 2020 15:21
@lucaong
Copy link
Contributor Author

lucaong commented May 22, 2020

I took care of rebasing on the current master, fixing the conflicts, and adapting this PR to the things that have changed since it was open 4 years ago.

The part dealing with the Rake version is obsolete and was removed.

Some context on the changes: the :sentinels and :role options were added to the DEFAULTS hash, so that the logic in _parse_options that copies over string keys to symbol equivalents takes care of those too. A few other places are adapted to make sure that also the inner sentinel options like :host, :port, or :password can indifferently be passed as symbols or strings, coherently with other options.

@alexpapworth
Copy link

Awesome! Can we get a member of the Redis team to review?

cc: @byroot

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

6 participants