-
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
List api test executor #328
Conversation
Fix scan commands (zio#321)
d14eef0
to
a187233
Compare
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.
Just a few minor comments, otherwise looks almost ready to go!
|
||
private[redis] final class TestExecutor private ( | ||
lists: TMap[String, Vector[String]], |
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 use Chunk instead of Vector.
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.
Ok changed to Chunk
private def errResponse(cmd: String): RespValue.BulkString = | ||
RespValue.bulkString(s"(error) ERR wrong number of arguments for '$cmd' command") | ||
|
||
private def onConnection(command: String, input: Chunk[RespValue.BulkString])( | ||
res: => RespValue.BulkString | ||
): USTM[BulkString] = STM.succeedNow(if (input.isEmpty) errResponse(command) else res) | ||
|
||
private[this] def runCommand(name: String, input: Chunk[RespValue.BulkString]): STM[RedisError, RespValue] = { | ||
private[this] def runCommand(name: String, input: Chunk[RespValue.BulkString]): USTM[RespValue] = { |
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 should make sure no unsafe calls are made, e.g. head on potentially empty chunk. It can (and should) be done in a separate pull request.
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 am not sure if I understand what you are trying to say here. From my point of view, we statically know which inputs we feed into each command and therefore we are able to decode the input even with the use of unsafe operations like head
etc. How do you imagine that we ensure that no unsafe calls are made
?
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.
What I meant is double-checking if we know statically (last time I took a look there were quite a few head calls), and if now, changing the type to NonEmptyChunk :). If there's nothing to be done here, it's a no-op (even better :) ).
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.
Alright got it. But as you said this can be done in a follow-up PR.
Co-authored-by: Dejan Mijić <dmijic@acm.org>
List api test executor (zio#328)
Closes: #195
Support all currently implemented List API Commands including the Blocking commands.
Two commands are missing in the API and thus not included in this PR. See : #324 #325