search

debasishg / scala-redis

A scala library for connecting to a redis server, or a cluster of redis nodes using consistent hashing on the client side.
1.02k stars 219 forks source link

remake SET implementation #250

Closed atais closed 5 years ago

atais commented 5 years ago

Fixing the issues:

debasishg commented 5 years ago

How about removing the currently deprecated version, deprecating the current version and adding the new one. Considering the number of users it will be too much to change the public API all on a sudden. Thanks a ton for all the hard work that u r putting into this but we need to be careful with breaking changes. I know many people use this library in production.

On Fri, 6 Sep 2019 at 2:35 AM, Michał Siatkowski notifications@github.com wrote:

@atais commented on this pull request.

In src/main/scala/com/redis/StringOperations.scala https://github.com/debasishg/scala-redis/pull/250#discussion_r321480863:

trait StringOperations extends StringApi { self: Redis =>

  • override def set(key: Any, value: Any)(implicit format: Format): Boolean =
  • send("SET", List(key, value))(asBoolean)
  • @deprecated("Use the more typesafe variant", "2.14")
  • def set(key: Any, value: Any, nxxx: Any, expx: Any, time: Long): Boolean = {
  • send("SET", List(key, value, nxxx, expx, time))(asBoolean)
  • }
  • override def set(key: Any, value: Any, onlyIfExists: Boolean, time: SecondsOrMillis): Boolean = {
  • val nxxx = if (onlyIfExists) "XX" else "NX"
  • val expx = time match {
  • case Seconds(v) => List("EX", v)
  • case Millis(v) => List("PX", v)
  • override def set(key: Any, value: Any, whenSet: SetBehaviour = Always, expire: Duration = null)
  • (implicit format: Format): Boolean = {

I was thinking about keeping old implementation or not and these are things that convinced me

  1. It is not a new feature, but a cleanup. You don't need to update if you like the old API
  2. The other implementations are back from 2010 - 2015. It is time to move on.
  3. It is just making more and more mess in the class and the project, I really see no point in keeping untyped version from 2013 or SecondsOrMilliseconds from 2015. Its 2019.

please reconsider. Or maybe it is worth making a branch 4.0 and we can merge it there.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/debasishg/scala-redis/pull/250?email_source=notifications&email_token=AAA2FX6WXKXZPMDTGG3H663QIFYCLA5CNFSM4IUCHIOKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCD23XWI#discussion_r321480863, or mute the thread https://github.com/notifications/unsubscribe-auth/AAA2FX4I2MEJHIZ56PGD7JLQIFYCLANCNFSM4IUCHIOA .

-- Sent from my iPhone

atais commented 5 years ago

@debasishg seems like a good tradeoff.

The default call (without the params) is cross-compatible. I have managed to base old API calls on the new one, so now you can remove it on major release without any problems.