Deprecate implicit materialization of Slf4jFactory

[armanbilge]

See #681 (comment). Since there is not one canonical LoggerFactory implementation, they should be passed explicitly, not implicitly.

Closes #681.

[armanbilge]

This would have been more convenient as an apply method, but unfortunately that's been squatted :(

[lorandszakacs]

I think it's good this way! cats.effect.std.Console is also instatiated using make instead of apply. Maybe we should rename this to make rather than create :D

[armanbilge]

Yes, agree now that we decided we are not totally deprecating implicit (otherwise, the apply was dead deprecated weight).

Regarding naming: I chose create based on this.

log4cats/slf4j/src/main/scala/org/typelevel/log4cats/slf4j/Slf4jLogger.scala

Line 38 in 4f02ef3

def create[F[_]: Sync](implicit name: LoggerName): F[SelfAwareStructuredLogger[F]] =

But I think make is good too 🤔 and maybe better to keep it independent of create.

[lorandszakacs]

To me LoggerFactory is akin to capability traits like cats.effect.std.Console and fs2.io.Network, and can be passed around implicitely as here in skunk.

It's just that I would highly encourage explicit instantiation of these by the user, and then allow them to chose if they wish to pass implicitely or explicitely in their own app :). I think the status quo is that Console is fine to be passed implicitely, right? If so, I'd reword the deprecation message on all methods to indicate that explicit instantation of a LoggerFactory of the user's choosing + the calling of methods directly on said LoggerFactory.

I'd also prefer to see this paired with some docs that showcase the explicit use of Slf4jFactory.create and passing it along to some classes (both implicitely and explicitely). Basically something like:

// the assorted imports, also code does not compile
object Main extends IOApp.Simple {
  override def run: IO[Unit] = for {
     implicit0(lf: LoggerFactory[IO]) = Slf4jFactory.create[IO]
     // other capabilities like cats.effect.std.{Console, Random}, etc here.
     app <- instantiateApp[IO]
  } yield ()
  
  type MyApp = Nothing

  def instantiateApp[F[_]: LoggerFactory]: F[MyApp] = 
   for {
      // many others in F
      someClass = new SomeClass[F]()
      // many others in F
   } yield ???
}

class SomeClass[F[_]](implicit loggers: LoggerFactory[F]){
   private val logger = loggers.getLogger //explicit call the LoggerFactory passed along by the user, even if implicit
}

LE: paired with updating this section of the docs which does rely on these implicits.

[lorandszakacs]

Maybe we should make some kind of integrative typelevel doc that showcases a pattern of instantiating capabilities available in the ecosystem and link that as an example? 😅

Since it would serve well to be consistent across everything under org.typelevel and people would also have a handle on why we do something like this in log4cats, fs2, cats-effect, etc.

[armanbilge]

To me LoggerFactory is akin to capability traits like cats.effect.std.Console and fs2.io.Network, and can be passed around implicitely as here in skunk.

I definitely agree those examples can be passed around implicitly :) that's because they have canonical implementations. Meaning that there is essentially only one true implementation.

• are you aware of any library providing an implementation of Console besides the one in Cats Effect? What would such an implementation do?
• Network is in fact sealed and therefore only FS2 can provide the one and only canonical implementation

This is not the case for LoggerFactory, and indeed would be quite inconvenient if it was :)

The fact that there are legitimate fears in #681 (review) that the wrong/unexpected implementation could be passed implicitly reinforces that this should be explicit.

---

I definitely agree the docs can/should be improved (indeed CI is failing about that). But first let's make sure we're all on the same page with this change :)

[armanbilge]

Ah, your comment in #681 (comment) clarifies things for me.

But I do not agree with encouraging people to not pass LoggerFactory implicitly, only with its automagic implicit creation.

That seems reasonable to me: we'd deprecate just this method and nothing more.

log4cats/slf4j/src/main/scala/org/typelevel/log4cats/slf4j/package.scala

Line 24 in d00181c

implicit def loggerFactoryforSync[F[_]](implicit F: Sync[F]): Slf4jFactory[F] =

Happy to hear other's thoughts :)

[lorandszakacs]

The fact that there are legitimate fears in #681 (review) that the wrong/unexpected implementation could be passed implicitly reinforces that this should be explicit.

Yes, but the fear is accidentally getting a new LoggerFactory created implicitely, not that it is passed around implicitely. I detailed the reasoning on this comment in #681. I'll copy paste the relevant bits.

At the end of the day how many users will have more than one instance of a LoggerFactory in their codebase? And if they do, there always various ways of newtype-ing, or subclassing it, etc. to solve the problem. For large codebases with literally hundreds of classes 90% of which do logging users will probably pass these around implicitly.

that's because they have canonical implementations

Well, there are good implementations of the capabilities. And I definitely would not be the one to try to implement a fs2.io.Network differently 😅. But they are hardly canonical in the way implementing Monad for Option is.

[lorandszakacs]

That seems reasonable to me: we'd deprecate just this method and nothing more.

Then we are in agreement 🥳

[bplommer]

To repeat my comment in #681 (comment) - Why not provide this as a convenience but in an implicits namespace, like we do with (for example) the cats-effect global runtime? That way it's not just "floating around out there" but it's available in a convenient form when needed.

[lorandszakacs]

Version needs to be bumped to 2.6.0 now 😅

[lorandszakacs]

To repeat my comment in #681 (comment) - Why not provide this as a convenience but in an implicits namespace, like we do with (for example) the cats-effect global runtime? That way it's not just "floating around out there" but it's available in a convenient form when needed.

@danicheg what do you think about this approach?

I'm fine going forward with this even without convenience .implicits import. But I'm not against it.

[danicheg]

To repeat my comment in #681 (comment) - Why not provide this as a convenience but in an implicits namespace, like we do with (for example) the cats-effect global runtime? That way it's not just "floating around out there" but it's available in a convenient form when needed.

@danicheg what do you think about this approach?

I think that Runtime abstraction and providing instances for LoggerFactory through the implicit scope – differ in their nature. But I'm biased in this question. I don't like the approach of using logging via capabilities.

[lorandszakacs]

Merging and we'll defer the discussion about implicit LoggerFactory when the ecosystem reaches more of a consensus about implicit capabilities in general 😅