search

gomodule / redigo

Go client for Redis
Apache License 2.0
9.75k stars 1.25k forks source link

feat: add GetWithContext in pool, returns ConnWithContext #629

Closed zhwei820 closed 2 years ago

zhwei820 commented 2 years ago

While activeConn implements ConnWithContext, I think it's better to return as ConnWithContext.

Besides, ConnWithContext is a superset of Conn, pool.Get remains unchanged and works perfectly.

Hi, in consideration of API compatibility, I added a new method GetWithContext in pool instance, returns ConnWithContext, remaining others untouched.

Fixed a bug in older tests code, and completed test code with GetWithContext method.

antsbean commented 2 years ago

In order to solve this problem ASAP, I suggest adding a new API GetContextV2 that doesn't break API compatibility

zhwei820 commented 2 years ago

While it's true that the returned type does implement ConnWithContext changing the return would break API compatibility for consumers.

For example if I had a interface which represented a pool including this method, that interface would not match requiring the consumer to update their code.

As such this type of change could only be introduced in a v2 release I'm afraid.

Hi, @stevenh, I renewed this PR. You may check it later.

zhwei820 commented 2 years ago

adding now seems unnecessary is the consumer can solve this easily with a little wrapper. Closed.

zhwei820 commented 2 years ago

Closed

zhwei820 commented 2 years ago

In the end, thank you for your maintenance of this awesome repo, and for quick and patient response. @stevenh 👍 👍

stevenh commented 2 years ago

No problem @zhwei820 thanks for the PR, hope you now have a good way forward.