-
-
Notifications
You must be signed in to change notification settings - Fork 44
feature: mapk for Meter #969
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the PR! I will review the changes this week. |
|
|
||
| import cats.Applicative | ||
| import cats.Functor | ||
| import cats.implicits.toFunctorOps |
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.
Makes things slightly more obvious:
| import cats.implicits.toFunctorOps | |
| import cats.syntax.functor._ |
We can apply the same change in all places.
|
Looks good overall, thanks! @NthPortal would you like to take a look? |
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.
this feature has a fundamental problem, which is that KindTransformers (perhaps better named LiftKind or KindLifter) do not (with the exception of the identity case) work in reverse.
as an example, one of the most common types for G is OptionT[F, *] (i.e. a wrapper around F[Option[*]]); you cannot unlift OptionT[F, *] back to F, because F[None] has no value to unwrap/unlift
| mct: MonadCancelThrow[F], | ||
| kt: KindTransformer[F, G], | ||
| tk: KindTransformer[G, F] |
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 some name bikeshedding:
- monadic constraints tend to just share the name of the HKT (idk why, but it's conventional)
ktis just the first letters of 'Kind' and 'Transformer', sotkdoesn't make a lot of sense as a name
| mct: MonadCancelThrow[F], | |
| kt: KindTransformer[F, G], | |
| tk: KindTransformer[G, F] | |
| F: MonadCancelThrow[F], | |
| ktFG: KindTransformer[F, G], | |
| ktGF: KindTransformer[G, F] |
| for { | ||
| state <- InMemoryMeterSharedState.create[IO](resource, scope, 0.seconds) | ||
| builder = SdkCounter.Builder[IO, Long](name, state.state, None, None) | ||
| mappedBuilder = builder.mapK[IO] |
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.
this is an identity transformation
|
|
||
| /** Modify the context `F` using an implicit [[KindTransformer]] from `F` to `G`. | ||
| */ | ||
| def mapK[G[_]: MonadCancelThrow](implicit |
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 think when both F ~> G and G ~> F are required, the method is more typically named imapK, e.g. in InvariantK from cats-tagless.
I've seen the implementation of mapK for Tracer and thought of doing the same for Meter.
I don't pretend that what I'm suggesting is a good implementation though. Also I didn't really get where is the best place to implement tests. So I did only an example one, for counter
Also, there is a point could be made that it is possible to make less amount of boilerplate code (in observable components at least) by encapsulating Builder trait
So, it's more of a feature suggestion
But if somebody will like the idea and will be able to tell me more about project structure then I'm ready to finish this pr