Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merge extra commands having different returning container types #44

Open
guersam opened this issue Aug 30, 2013 · 10 comments
Open

Merge extra commands having different returning container types #44

guersam opened this issue Aug 30, 2013 · 10 comments

Comments

@guersam
Copy link
Collaborator

guersam commented Aug 30, 2013

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
Copy link
Collaborator Author

guersam commented Aug 30, 2013

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
Copy link
Collaborator Author

guersam commented Aug 30, 2013

And ZRangeReturn should be parameterized as well.

@debasishg
Copy link
Owner

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

@guersam
Copy link
Collaborator Author

guersam commented Aug 30, 2013

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

@debasishg
Copy link
Owner

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

@guersam
Copy link
Collaborator Author

guersam commented Aug 30, 2013

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?

@debasishg
Copy link
Owner

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.

@guersam
Copy link
Collaborator Author

guersam commented Aug 30, 2013

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.

@andyscott
Copy link
Contributor

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.

@debasishg
Copy link
Owner

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants