scala-redis-nb icon indicating copy to clipboard operation
scala-redis-nb copied to clipboard

Merge extra commands having different returning container types

Open guersam opened this issue 10 years ago • 10 comments

Currently we have extra case classes like ZRangeWithScores to support for commands with varying return types according to it's argument.

We might merge them into original commands with path-dependent type like this:

sealed trait ZRangeReturn { type Out }
case class Default[A: Reader]() extends ZRangeReturn { type Out = List[A] }
case class WithScores[A: Reader]() extends ZRangeReturn { type Out = List[(A, Double)] }

I'll give it a try when time allows.

guersam avatar Aug 30 '13 00:08 guersam

The problem is that we should always use WithScores() instead of just WithScores because the latter is evaluated as WithScores.type, not the instance.

A solution is to use implicit conversion from Boolean:

object ZRangeReturn {
  implicit def apply[A: Render](withScores: Boolean): ZrangeReturn =
    if (withScores) WithScores[A]() else Default[A]()
}

guersam avatar Aug 30 '13 02:08 guersam

And ZRangeReturn should be parameterized as well.

guersam avatar Aug 30 '13 02:08 guersam

How about returning an Either[List[A], List[(A, Double)]] from ZRangeReturn ?

debasishg avatar Aug 30 '13 06:08 debasishg

I'm not sure where to put it. Would you give me an insight?

guersam avatar Aug 30 '13 06:08 guersam

oops .. I meant ZRange to return an Either depending on withScores as a boolean argument defaulted to false.

debasishg avatar Aug 30 '13 06:08 debasishg

I see. It will let user know the return type more explicltly.

IMHO, however, it would require additional steps from both of us (having another special deserializer) and users (should work on projection). Can it be sufficient with proper API documentation about the return type?

guersam avatar Aug 30 '13 07:08 guersam

Currently I think proper API documentation for the return type is fine. Just wanted to raise it as a question to see what others feel about it. Yes, we need a de-serializer but that should not be too difficult. But how do you feel about Either being the return type. Does it make an API user feel that some things are being forcefully grouped together ? Or we can have the Either based single abstraction at the command level and have separate APIs at the operations level - a slight repetition but the API becomes more granular but possibly user-friendly . This also goes in line w/ the earlier thoughts of thinking commands as implementation artifacts and may not have a 1:1 correspondence with the APIs.

debasishg avatar Aug 30 '13 07:08 debasishg

Agree on your point. As these are tradeoffs and it's not urgent, let's just keep it open for now and see if there's anyone interested in.

guersam avatar Aug 30 '13 07:08 guersam

The current solution has the advantage of having a specific return type at both the command and operation level. It sounds like merging the case classes would mean adding another layer of complexity to the return type processing, which I'm not particularly fond of doing. Also, I generally use Either as a return type when I expect the left hand side to contain any errors if the command failed.

andyscott avatar Aug 30 '13 19:08 andyscott

@andyscott +1 on your thoughts on simplicity of the current implementation. The alternative was an approach towards DRY. Regarding Either, scala.Either is not left biased and hence I proposed it. If I need something which has a left bias towards Exception, I usually go for scala.util.Try or scalaz.\/.

So as @guersam suggested let's keep it open for the time being.

debasishg avatar Aug 30 '13 19:08 debasishg