[548] Expose Redis operations via Redis trait #727
Conversation
|
Am I missing something? This PR seems to contain only benchmarks. |
|
@mijicd you scared me there a bit :) but i see all the changes, not just in the benchmark module |
I guess it's GitHub app issue. I'll check it out from laptop. |
| private[redis] def run(in: In)(implicit executor: RedisExecutor, codec: BinaryCodec): ZIO[Any, RedisError, Out] = | ||
| executor | ||
| .execute(resp(in, codec)) | ||
| .flatMap[Any, Throwable, Out](out => ZIO.attempt(output.unsafeDecode(out)(codec))) | ||
| .refineToOrDie[RedisError] | ||
|
|
There was a problem hiding this comment.
If we're going with implicits, then:
- let's keep the implicits alphabetically ordered
- let's use an alias -
IO[RedisError, Out]as return type (this is also applicable to the commands) - let's remove the other
run
Otherwise, we can try passing codec and executor as constructor parameters here.
There was a problem hiding this comment.
I don't insist on implicits, it was just a way to get things done faster. I can make them "explicit" args or constructor params. Makes probably more sense as a constructor param as you suggested. Any particular order? Second argument list?
There was a problem hiding this comment.
Sure. It probably makes sense grouping it that, but I'm also fine if you place them in the same one.
There was a problem hiding this comment.
i made them constructor params in second argument list. Only complication was AskingCommand. I resolved it but not sure if it's the best way.
| trait CommandExecutor { | ||
| implicit def codec: BinaryCodec | ||
|
|
||
| implicit def executor: RedisExecutor | ||
| } |
There was a problem hiding this comment.
Let's rename it somehow, as this is not really an executor.
There was a problem hiding this comment.
yeah i was struggling with naming this one, considering those two methods were making Redis trait before this change. could it be something like RedisEnvironment or CommandExecutable. Any suggestion?
There was a problem hiding this comment.
Environment sounds good. They don't need to be implicit anymore either. The question remains: can we remove it completely?
There was a problem hiding this comment.
i don't think we can, unless we want to split command creation and execution. If we changed API traits to return RedisCommand and pushed invocation of run then we wouldn't need these dependencies on those traits. Or do you have some ideas?
There was a problem hiding this comment.
I would not "expose" run. As mentioned below, 2nd parameter group is a bit too much as well, imo. On the other side, it makes sense if we restore implicits. 🤔
There was a problem hiding this comment.
i mean't something like zio-dynamodb does, where you wrap command and arguments in some internal class which exposes method like
def execute: ZIO[RedisExecutor & BinaryCodec, RedisError, Out]but that's different issue imho
There was a problem hiding this comment.
That's not bad. We might be able to combine it with returning somehow and make a nice "proxy", but let's leave it for the follow-up PR (if you're willing to dig into it :) ).
|
|
||
| import zio.schema.codec.BinaryCodec | ||
|
|
||
| trait RedisEnvironment { |
There was a problem hiding this comment.
Maybe we should make it package-private. After all, it is an implementation detail.
| executor: RedisExecutor, | ||
| codec: BinaryCodec |
There was a problem hiding this comment.
I'm not sure about this parameter group. Let's merge them in the first one (sorry 🙏 )
|
@m-kalai Please adjust the docs examples and it's ready to go. |
Resolves #548
Redisenvironment dependency from all the API traitsRedistraitredispackage object and fixing tests