-
Notifications
You must be signed in to change notification settings - Fork 80
SAM Type Interface #920
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?
SAM Type Interface #920
Conversation
core/shared/src/main/scala/org/typelevel/log4cats/JsonLike.scala
Outdated
Show resolved
Hide resolved
final case class KernelLogLevel(name: String, value: Double) { | ||
def namePadded: String = KernelLogLevel.padded(this) | ||
|
||
KernelLogLevel.add(this) | ||
} |
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.
issue (structure): I'm sick right now, but we should get on a call towards the end of next week and go over bincompat.
Case classes are wonderful for applications, but we're going to tend to avoid them because they don't play nicely with bincompat.
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.
agreed 💯
core/shared/src/main/scala/org/typelevel/log4cats/KernelLogLevel.scala
Outdated
Show resolved
Hide resolved
def add(level: KernelLogLevel): Unit = synchronized { | ||
val length = level.name.length | ||
map += level.name.toLowerCase -> level | ||
if (length > maxLength) { | ||
maxLength = length | ||
padded = map.map { case (_, level) => | ||
level -> level.name.padTo(maxLength, ' ').mkString | ||
} | ||
} else { | ||
padded += level -> level.name.padTo(maxLength, ' ').mkString | ||
} | ||
} |
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.
issue: I'm not convinced we need to support dynamically adding log levels, or looking them up by name.
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.
Reminder to remove the dynamically added log levels
core/shared/src/main/scala/org/typelevel/log4cats/KernelLogLevel.scala
Outdated
Show resolved
Hide resolved
final def logTrace(record: Log.Builder => Log.Builder): F[Unit] = | ||
log(KernelLogLevel.Trace, record) | ||
final def logDebug(record: Log.Builder => Log.Builder): F[Unit] = | ||
log(KernelLogLevel.Debug, record) | ||
final def logInfo(record: Log.Builder => Log.Builder): F[Unit] = | ||
log(KernelLogLevel.Info, record) | ||
final def logWarn(record: Log.Builder => Log.Builder): F[Unit] = | ||
log(KernelLogLevel.Warn, record) | ||
final def logError(record: Log.Builder => Log.Builder): F[Unit] = | ||
log(KernelLogLevel.Error, record) |
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.
suggestion (naming): If we do end up needing these, we should simplify down the names
final def logTrace(record: Log.Builder => Log.Builder): F[Unit] = | |
log(KernelLogLevel.Trace, record) | |
final def logDebug(record: Log.Builder => Log.Builder): F[Unit] = | |
log(KernelLogLevel.Debug, record) | |
final def logInfo(record: Log.Builder => Log.Builder): F[Unit] = | |
log(KernelLogLevel.Info, record) | |
final def logWarn(record: Log.Builder => Log.Builder): F[Unit] = | |
log(KernelLogLevel.Warn, record) | |
final def logError(record: Log.Builder => Log.Builder): F[Unit] = | |
log(KernelLogLevel.Error, record) | |
final def trace(record: Log.Builder => Log.Builder): F[Unit] = | |
log(KernelLogLevel.Trace, record) | |
final def debug(record: Log.Builder => Log.Builder): F[Unit] = | |
log(KernelLogLevel.Debug, record) | |
final def info(record: Log.Builder => Log.Builder): F[Unit] = | |
log(KernelLogLevel.Info, record) | |
final def warn(record: Log.Builder => Log.Builder): F[Unit] = | |
log(KernelLogLevel.Warn, record) | |
final def error(record: Log.Builder => Log.Builder): F[Unit] = | |
log(KernelLogLevel.Error, record) |
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.
lets keep it until final change, I tried to change as suggested but its giving error due to duplicates
js-console/src/main/scala/org/typelevel/log4cats/console/ConsoleLogger.scala
Outdated
Show resolved
Hide resolved
Thank you for all the suggestions, I will rework on it |
// For Java/legacy interop, if needed (not implicit) | ||
val LevelOrdering: Ordering[KernelLogLevel] = | ||
Ordering.by[KernelLogLevel, Int](_.value).reverse |
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.
todo: Java interop is a non-issue, since attempting to use cats-effect from Java would be painful (at best) and the idioms are all sorts of incompatible.
There's an implicit which provides an Ordering[A]
if there is an Order[A]
in scope, so this can be removed.
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.
Reminder to remove this
override def error(t: Throwable)(message: => String): F[Unit] = l.error(t)(f(message)) | ||
} | ||
|
||
private def mapK[G[_], F[_]](f: G ~> F)(logger: SamStructuredLogger[G]): SamStructuredLogger[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.
suggestion: This is going to need to be overridden in a bunch of Logger
subclasses, so you'll probably want to add a .mapK
to LoggerKernel
implicit def optionTSamStructuredLogger[F[_]: Functor](implicit | ||
ev: SamStructuredLogger[F] | ||
): SamStructuredLogger[OptionT[F, *]] = | ||
ev.mapK(OptionT.liftK[F]) | ||
|
||
implicit def eitherTSamStructuredLogger[F[_]: Functor, E](implicit | ||
ev: SamStructuredLogger[F] | ||
): SamStructuredLogger[EitherT[F, E, *]] = | ||
ev.mapK(EitherT.liftK[F, E]) | ||
|
||
implicit def kleisliSamStructuredLogger[F[_], A](implicit | ||
ev: SamStructuredLogger[F] | ||
): SamStructuredLogger[Kleisli[F, A, *]] = | ||
ev.mapK(Kleisli.liftK[F, A]) |
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.
issue: We really don't want to add implicit conversions for this sort of thing. It leads to really confusing code.
// Use simple timestamp formatting instead of java.time.Instant for Scala Native compatibility | ||
val timeStr = s"${new java.util.Date(timestamp).toString}" |
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.
cleanup: Scala Native should support Instant, and it's better to avoid the java.util
time classes whenever possible.
val kernel = NoOpLoggerKernel[IO, String] | ||
val logger = SamLogger.wrap(kernel) | ||
// All of these should be no-ops and not throw | ||
logger.info("This should not appear").void *> |
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.
issue: This isn't actually testing what the comment hint it's testing.
To do that, you'd need something like this:
private def boom()(implicit loc: munit.Location): String = fail("This code should not have executed")
This would replace the message.
logger.info("This should not appear").void *> | |
logger.info(boom()).void *> |
// All of these should be no-ops and not throw | ||
logger.info("This should not appear").void *> | ||
logger.error("This should not appear").void *> | ||
logger.warn("This should not appear").void *> |
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.
cleanup: logger.warn
and friends already return F[Unit]
, so you don't need to add .void
import cats.effect.IO | ||
import munit.CatsEffectSuite | ||
|
||
class SamLoggerTest extends CatsEffectSuite { |
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.
issue: tests that require you to inspect the console to see if they pass or not aren't automated tests.
They can be useful when doing exploratory programming, but they shouldn't generally be committed to source.
test("SamStructuredLogger should handle context with multiple entries") { | ||
var capturedLogs: List[(KernelLogLevel, Log.Builder[String] => Log.Builder[String])] = Nil | ||
|
||
val testKernel = new LoggerKernel[IO, String] { |
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.
issue: Instead of inlining this with a var
, it'd be better to rework the existing test loggers to use LoggerKernel
…khande/log4cats into feature/log4cats-sam-type
This PR's objective is to create a SAM type interface in log4cats