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

Laziness #3

Closed
rossabaker opened this issue May 12, 2022 · 3 comments · Fixed by #22
Closed

Laziness #3

rossabaker opened this issue May 12, 2022 · 3 comments · Fixed by #22

Comments

@rossabaker
Copy link
Member

We are paying to calculate the attributes even when the implementation is noop. Can we do better? Something like what log4s / log4cats do with macros?

@iRevive iRevive mentioned this issue Jun 18, 2022
@iRevive
Copy link
Contributor

iRevive commented Jun 18, 2022

I've solved a similar issue with overloading:

trait Trace[F[_]] {
  protected def spanInternal[A](name: => String, params: => Seq[TraceParam], fa: F[A]): F[A]

  final def span[A](name: => String)(fa: F[A]): F[A] =
    spanInternal(name, Nil, fa)

  final def span[A](name: => String, param1: => TraceParam)(fa: F[A]): F[A] =
    spanInternal(name, Seq(param1), fa)

  final def span[A](name: => String, param1: => TraceParam, param2: => TraceParam)(fa: F[A]): F[A] =
    spanInternal(name, Seq(param1, param2), fa)

  // and so on
}

This option just works, but it's far away from the perfect solution. I guess macros is more suitable.

scala-logging has some good example, both in scala 2 and scala 3: https://github.com/lightbend/scala-logging/blob/main/src/main/scala-3/com/typesafe/scalalogging/LoggerMacro.scala

@rossabaker
Copy link
Member Author

I'm most familiar with log4s, but it's similar to LoggerMacro. I suspect log4cats is the closest to what we need to do. It's similar to those other two, but its return types are effectful, like ours. Pretty sure our macros would need some F.unit instead of ().

I hate to introduce this complexity, but I strongly suspect it's going to be worthwhile if we want to get this into major libraries and not worsen the experience.

@iRevive iRevive linked a pull request Jul 3, 2022 that will close this issue
@iRevive
Copy link
Contributor

iRevive commented Oct 20, 2023

Resolved by #22

@iRevive iRevive closed this as completed Oct 20, 2023
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 a pull request may close this issue.

2 participants