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.

Sign up for GitHub

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

Jump to bottom

Deprecate implicit materialization of Slf4jFactory #683

Merged
merged 6 commits into from
Sep 20, 2022
Merged

Deprecate implicit materialization of Slf4jFactory #683

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d00181c
Deprecate implicit use of `LoggerFactory`
armanbilge Aug 23, 2022
c515035
Revert "Deprecate implicit use of `LoggerFactory`"
armanbilge Aug 23, 2022
64ce5e5
Only deprecated `loggerFactoryforSync`
armanbilge Aug 23, 2022
fe1883a
Update docs
armanbilge Aug 23, 2022
6c60b66
Fix typo
armanbilge Aug 23, 2022
93aa9ba
bump deprecation version
armanbilge Sep 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 3 additions & 15 deletions docs/index.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,29 +100,17 @@ This brings several advantages:
If you are unsure how to create a new `LoggerFactory[F]` instance, then you can look at the `log4cats-slf4j`,
or `log4cats-noop` modules for concrete implementations.

The quickest fix might be to import needed implicits:
```scala mdoc:silent
// assumes dependency on log4cats-slf4j module
import org.typelevel.log4cats._
import org.typelevel.log4cats.slf4j._

val logger: SelfAwareStructuredLogger[IO] = LoggerFactory[IO].getLogger

// or
def anyFSyncLogger[F[_]: Sync]: SelfAwareStructuredLogger[F] = Slf4jFactory[F].getLogger
```

Alternatively, a mutually exclusive solution is to explicitly create your
`LoggerFactory[F]` instance and pass them around implicitly:
The quickest fix might be to create an instance of a `Slf4jFactory` and pass it around implicitly:
```scala mdoc:reset:silent
import cats.effect.IO
import cats.Monad
import cats.syntax.all._
import org.typelevel.log4cats._
// assumes dependency on log4cats-slf4j module
import org.typelevel.log4cats.slf4j.Slf4jFactory

// create our LoggerFactory
implicit val logging: LoggerFactory[IO] = Slf4jFactory[IO]
implicit val logging: LoggerFactory[IO] = Slf4jFactory.create[IO]

// we summon LoggerFactory instance, and create logger
val logger: SelfAwareStructuredLogger[IO] = LoggerFactory[IO].getLogger
Expand Down
15 changes: 15 additions & 0 deletions slf4j/src/main/scala/org/typelevel/log4cats/slf4j/Slf4jFactory.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package org.typelevel.log4cats
package slf4j

import cats.effect.kernel.Sync
import org.slf4j.{Logger => JLogger}

trait Slf4jFactory[F[_]] extends LoggerFactory[F] {
Expand All @@ -28,6 +29,20 @@ trait Slf4jFactory[F[_]] extends LoggerFactory[F] {
object Slf4jFactory extends LoggerFactoryGenCompanion {
def apply[F[_]: Slf4jFactory]: Slf4jFactory[F] = implicitly

def create[F[_]: Sync]: Slf4jFactory[F] = new Slf4jFactory[F] {
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

override def getLoggerFromName(name: String): SelfAwareStructuredLogger[F] =
Slf4jLogger.getLoggerFromName(name)

override def getLoggerFromSlf4j(logger: JLogger): SelfAwareStructuredLogger[F] =
Slf4jLogger.getLoggerFromSlf4j(logger)

override def fromName(name: String): F[SelfAwareStructuredLogger[F]] =
Slf4jLogger.fromName(name)

override def fromSlf4j(logger: JLogger): F[SelfAwareStructuredLogger[F]] =
Slf4jLogger.fromSlf4j(logger)
}

def getLoggerFromSlf4j[F[_]](logger: JLogger)(implicit
lf: Slf4jFactory[F]
): SelfAwareStructuredLogger[F] = lf.getLoggerFromSlf4j(logger)
Expand Down
16 changes: 3 additions & 13 deletions slf4j/src/main/scala/org/typelevel/log4cats/slf4j/package.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,20 +17,10 @@
package org.typelevel.log4cats

import cats.effect.Sync
import org.slf4j.{Logger => JLogger}

package object slf4j {
implicit def loggerFactoryforSync[F[_]: Sync]: Slf4jFactory[F] = new Slf4jFactory[F] {
override def getLoggerFromName(name: String): SelfAwareStructuredLogger[F] =
Slf4jLogger.getLoggerFromName(name)

override def getLoggerFromSlf4j(logger: JLogger): SelfAwareStructuredLogger[F] =
Slf4jLogger.getLoggerFromSlf4j(logger)

override def fromName(name: String): F[SelfAwareStructuredLogger[F]] =
Slf4jLogger.fromName(name)

override def fromSlf4j(logger: JLogger): F[SelfAwareStructuredLogger[F]] =
Slf4jLogger.fromSlf4j(logger)
}
@deprecated("Use Slf4jFactory.create[F] explicitly", "2.5.0")
Copy link
Member

Choose a reason for hiding this comment

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

Version needs to be bumped to 2.6.0 now 😅

armanbilge marked this conversation as resolved.
Show resolved Hide resolved
implicit def loggerFactoryforSync[F[_]: Sync]: Slf4jFactory[F] =
Slf4jFactory.create[F]
}
2 changes: 1 addition & 1 deletion slf4j/src/test/scala/org/typelevel/log4cats/slf4j/Slf4jLoggerMacroCompilationTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ class Slf4jLoggerOuterClassMacroTest {

object LoggingBaseline {
val t = new Throwable
def logger[F[_]: Sync]: SelfAwareStructuredLogger[F] = Slf4jFactory.getLogger[F]
def logger[F[_]: Sync]: SelfAwareStructuredLogger[F] = Slf4jFactory.create[F].getLogger

val traceM = logger[IO].trace("")
val traceTM = logger[IO].trace(t)("")
Expand Down