-
Notifications
You must be signed in to change notification settings - Fork 64
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
Lpos #331
Lpos #331
Conversation
@@ -348,6 +348,16 @@ object Input { | |||
} | |||
} | |||
|
|||
case object ListMaxLenInput extends Input[ListMaxLen] { | |||
override private[redis] def encode(data: ListMaxLen): Chunk[RespValue.BulkString] = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove override private prefix, just to align it with othe inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -184,6 +184,21 @@ object Output { | |||
} | |||
} | |||
|
|||
case object OptionalLongOrLongArrayOutput extends Output[Either[Option[Long], Chunk[Long]]] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We introduced a convention recently to provide separate commands based on the output so that we don't force pattern match on the call site. Check out geo commands for an example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea I also thought about that but decided to keep it as close to the Redis API as possible. I'll the geo commands and split the LPOS into two commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done Split into Lpos
and LposCount
commands
@@ -75,7 +75,7 @@ trait Streams { | |||
|
|||
type NoAck = NoAck.type | |||
|
|||
case class MaxLen(approximate: Boolean, count: Long) | |||
case class StreamMaxLen(approximate: Boolean, count: Long) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not necessary related to this PR, but I'd make these sealed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done and made a lot of other case classes sealed. Can you maybe explain to me why the compiler refused to accept final
case classes here that would even be better than just sealed or do I get something wrong?
Closes: #325
Implementation of the List API LPOS command, and tests and support for the TestExecutor