Skip to content

Conversation

@fvaresi
Copy link

@fvaresi fvaresi commented Feb 12, 2018

@makasim
Copy link
Member

makasim commented Feb 13, 2018

It would be better to upgrade the constructor of RedisConnectionFactory so that it accepts an instance of Redis object.

wdyt?

@fvaresi
Copy link
Author

fvaresi commented Feb 13, 2018

@makasim ok, will give that a try.

@fvaresi
Copy link
Author

fvaresi commented Feb 14, 2018

Hi @makasim. I was reviewing the code and I think handling this as a config option is better than modifying the constructor of RedisConnectionFactory for the following reasons:

a) if we modify RedisConnectionFactory, we won't be able to continue using the RedisTransportFactory abstraction without significant modifications.
b) one could still use RedisConnectionFactory directly, but this would make it difficult to switch to a different transport in the future
c) if we use the proposed changes in config handling instead, SimpleClient would still work without any changes.

What do you think? Do you still think modifying RedisConnectionFactory is the best option?

@makasim
Copy link
Member

makasim commented Feb 15, 2018

there is a better solution #367 (comment)

@makasim makasim closed this Feb 15, 2018
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.

2 participants