feat(redis): add NewWithClient to work with pre-existing client #137
Conversation
glimchb
force-pushed
the
redis-new-client
branch
2 times, most recently
from
1d7f2d5 to
3423387
Compare
Codecov Report
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. @@ Coverage Diff @@
## master #137 +/- ##
==========================================
+ Coverage 64.60% 64.84% +0.23%
==========================================
Files 24 25 +1
Lines 2057 2085 +28
==========================================
+ Hits 1329 1352 +23
- Misses 601 606 +5
Partials 127 127
... and 2 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
philippgille
left a comment
philippgille
left a comment
There was a problem hiding this comment.
I was looking forward to these constructors to give users maximum flexibility while keeping our library surface area small 👍
Just two comments. And if #130 is merged first, a change to the Godoc here might be required to mention the timeout option (in addition to the codec, as applicable options)
| if options.Codec == nil { | ||
| options.Codec = DefaultOptions.Codec | ||
| } | ||
| return &Client{c: rc, codec: options.Codec} |
There was a problem hiding this comment.
The NewClient currently includes a Ping call to check/establish a connection. Maybe we can do the same here, to minimize NewClient vs NewWithClient differences?
|
Ah and P.S.: I think we should also extend the tests to make use of a client created with this new constructor |
glimchb
force-pushed
the
redis-new-client
branch
from
3423387 to
20b72c5
Compare
Signed-off-by: Boris Glimcher <[email protected]>
glimchb
force-pushed
the
redis-new-client
branch
from
20b72c5 to
2ab4b47
Compare
3 participants
Signed-off-by: Boris Glimcher [email protected]