Addressing points raised in detektJvmMain task #203
Conversation
… control flow" in KLoggerNameResolver.unwrapCompanionClass(); Consolidated MDC cleanup code in KotlinLoggingMDC
|
@oshai Is there something I need to fix in the PR so that the |
|
I think it's related to this issue, I will just ignore it. |
|
I will review it as soon as I have some time. |
| val previousMDC = if (restorePrevious) { | ||
| pairForMDC.map { (k, _) -> | ||
| k to MDC.get(k) | ||
| val cleanupCallbacks = pairForMDC.map { (mdcKey, _) -> |
There was a problem hiding this comment.
This code consolidates the cleanup logic in one place. Instead having an if-condition for restorePrevious both before and after the execution of the body payload, it's now only checked before the payload execution; afterwards, there's only one flow path: cleanupCallbacks executes whatever cleanup code it's been assigned.
| @@ -91,7 +82,14 @@ public inline fun <T> withLoggingContext( | |||
| restorePrevious: Boolean = true, | |||
| body: () -> T | |||
| ): T { | |||
| val previousMDC = map.mapValues { MDC.get(it.key) } | |||
| val cleanupCallbacks = map.map { | |||
| } | ||
| } | ||
| return clazz | ||
| return clazz.enclosingClass?.let { enclosingClass -> |
There was a problem hiding this comment.
I also don't see a good reason to change that
There was a problem hiding this comment.
Searching through declaredFields instead of using getDeclaredField() has the advantage of not throwing NoSuchFieldException if the declared field cannot be found. If there's a Kotlin-implemented version of this functionality that doesn't use reflection and handles the "field not found" case more gracefully than the Java-implemented version, I'd be glad to use that, but in general, it's better to avoid using exceptions as control flow when possible.
| @@ -558,7 +559,7 @@ internal class LocationAwareKLogger(override val underlyingLogger: LocationAware | |||
| override fun entry(vararg argArray: Any?) { | |||
| if (underlyingLogger.isTraceEnabled(ENTRY)) { | |||
| val tp = MessageFormatter.arrayFormat(buildMessagePattern(argArray.size), argArray) | |||
| underlyingLogger.log(ENTRY, fqcn, LocationAwareLogger.TRACE_INT, tp.message, null, null); | |||
| @@ -182,4 +182,47 @@ class KotlinLoggingMDCTest { | |||
| assertNull(MDC.get("f")) | |||
| assertEquals("l", MDC.get("k")) | |||
| } | |||
|
|
|||
| @Test | |||
…nClass(); Consolidated testing code for mapping with logging context in KotlinLoggingMDCTest
|
Thanks! |
Removed "exception as control flow" in KLoggerNameResolver.unwrapCompanionClass(); Consolidated MDC cleanup code in KotlinLoggingMDC