-
Notifications
You must be signed in to change notification settings - Fork 63
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
[WIP] exposing RedisEnvironment via environment #739
Conversation
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.
A few minor comments from my side, but we have a bigger problem, which is that RedisEnvironment
(or BinaryCodec with RedisExecutor
) leaks in user-land, thus going against the service pattern and the changes you did in #727.
lazy val layer: ULayer[Redis] = | ||
ZLayer.succeed(Redis) |
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.
Well, you may as well remove the RedisLive
object and pack everything in Redis
, e.g.:
object Redis {
lazy val layer: ULayer[Redis] = ZLayer.succeed(new Redis)
}
In the future we may consider renaming it to live
, perhaps it'll be more consistent with the ecosystem.
lazy val layer = ZLayer { | ||
for { | ||
codec <- ZIO.service[BinaryCodec] | ||
executor <- ZIO.service[RedisExecutor] | ||
} yield new RedisEnvironment(codec, executor) | ||
} |
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.
Super nitpick:
lazy val layer = ZLayer { | |
for { | |
codec <- ZIO.service[BinaryCodec] | |
executor <- ZIO.service[RedisExecutor] | |
} yield new RedisEnvironment(codec, executor) | |
} | |
lazy val layer = | |
ZLayer { | |
for { | |
codec <- ZIO.service[BinaryCodec] | |
executor <- ZIO.service[RedisExecutor] | |
} yield new RedisEnvironment(codec, executor) | |
} |
def executor: RedisExecutor | ||
final case class RedisEnvironment(codec: BinaryCodec, executor: RedisExecutor) | ||
|
||
private[redis] object RedisEnvironment { |
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'm wondering if this should be private at all now that it's a part of all commands. In fact, I'm thinking if it would be wise to replace it with BinaryCodec with RedisExecutor
.
yes you're right, i was just thinking that maybe it's ok to expose codec and executor to user. saw some libs doing that, but yes it doesn't play well with service pattern. to follow service pattern exactly, we should move implementation of commands from trait to some class and pass codec and executor as constructor params. if we don't want to mix codec and executor into Redis trait, or expose them to the user via environment, or depend on use of implicits, there is probably no other way. or we can just leave it as is. |
codec
andexecutor
dependency fromRedisCommand
creation to it's execution