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
Fix cluster connecting option bug #889
Fix cluster connecting option bug #889
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two very minor comments, LGTM otherwise.
lib/redis/cluster.rb
Outdated
[Node.new(option.per_node_key, node_flags, option.use_replica?), | ||
Slot.new(available_slots, node_flags, option.use_replica?)] | ||
ensure | ||
node.map(&:disconnect) | ||
node&.map(&:disconnect) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick, but since the return values doesn't matter, .each
would make sense here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 584a8c6
lib/redis/cluster/option.rb
Outdated
def add_common_node_option_if_needed(options, node_opts, key) | ||
return options if options[key].nil? && node_opts.first[key].nil? | ||
|
||
options[key] = options[key] || node_opts.first[key] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use ||=
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed 584a8c6
oh... Ruby 2.3 is EOL but I will accommodate |
Thanks! |
Thank you for your quick and polite reviewing. |
This is related to #888.
I modified it as keeping node connecting option as Hash instead of URI.