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

Add imapK and instrument helpers #772

Closed
wants to merge 1 commit into from

Conversation

ybasket
Copy link
Contributor

@ybasket ybasket commented Feb 1, 2023

Add ways to transform instances of RedisCommands with regard to their effect type. This enables to work more easily with Kleisli-based effects or collect metrics like epimetheus-redis4cats does and breaks whenever new operations are added to redis4cats.

Add ways to transform instances of RedisCommands with regard to their effect type. This enables to work more easily with Kleisli-based effects or collect metrics like epimetheus-redis4cats does and breaks whenever new operations are added to redis4cats.

implicit class InstrumentOps[F[_], K, V](val cmd: RedisCommands[F, K, V]) extends AnyVal {
def instrument(ff: F ~> F, includeTransactions: Boolean): RedisCommands[F, K, V] = {
if(includeTransactions)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirely sure whether both use cases actually exist or whether an opinionated version would be sufficient. Maybe someone has more insights here (I'm not using transactions/pipelines)?

Copy link
Member

@gvolpe gvolpe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @ybasket , sorry to turn you down, but the code duplication of every Redis method is what we've been avoiding since the creation of this project.

I assume the liftK extension method is not sufficient for your use case? I'd be happy to accept imapK if we can get rid of all the duplication, as that's some heavy maintenance burden I would rather avoid.

If that isn't feasible, I would propose another repository for this functionality (e.g. profunktor-redis4cats-ops) with a different release cycle, which should work with package-private code.

@ybasket
Copy link
Contributor Author

ybasket commented Feb 3, 2023

Hey @ybasket , sorry to turn you down, but the code duplication of every Redis method is what we've been avoiding since the creation of this project.

I assume the liftK extension method is not sufficient for your use case? I'd be happy to accept imapK if we can get rid of all the duplication, as that's some heavy maintenance burden I would rather avoid.

If that isn't feasible, I would propose another repository for this functionality (e.g. profunktor-redis4cats-ops) with a different release cycle, which should work with package-private code.

Hey, no problem, I was aware of the risk this could happen. It's not that I like the repetition, it would just move some issues to a place where the solution is kept more in sync. I guess an ops repository would suffer the same pain as epimetheus-redis4cats now (see davenverse/epimetheus-redis4cats#96).

liftK is unfortunately not enough for all use cases mentioned, unless you fiddle with a customised Async – something I wouldn't dare. There might be an alternative way to do it via the FutureLift abstraction, toying around with allowing some customisation in there. Will let you know in case this evolves to something useful.

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

Successfully merging this pull request may close these issues.

2 participants