Add coil3 extension - #479
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a new kermit-coil3 extension module so Coil 3’s logging can be routed into Kermit, plus the necessary Gradle wiring to include/publish the new extension.
Changes:
- Register the new
:kermit-coil3Gradle module and add Coil 3 to the version catalog. - Introduce
KermitCoil3Loggerimplementing Coil 3’sLogger, plus level/Severity conversion extensions. - Add module build configuration, public API dumps, and end-user README documentation.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| settings.gradle.kts | Includes the new :kermit-coil3 project and maps it to extensions/kermit-coil3. |
| gradle/libs.versions.toml | Adds Coil 3 version and coil3-core library alias for dependency management. |
| extensions/kermit-coil3/src/commonMain/kotlin/co/touchlab/kermit/coil3/KermitCoil3Logger.kt | Adds a Coil 3 Logger implementation backed by a Kermit Logger. |
| extensions/kermit-coil3/src/commonMain/kotlin/co/touchlab/kermit/coil3/Extensions.kt | Adds public conversion extensions between Coil 3 Logger.Level and Kermit Severity. |
| extensions/kermit-coil3/README.md | Documents how to wire KermitCoil3Logger into a Coil 3 ImageLoader. |
| extensions/kermit-coil3/build.gradle.kts | Adds the new KMP extension module build/publish configuration and targets. |
| extensions/kermit-coil3/api/jvm/kermit-coil3.api | Adds JVM API dump for binary compatibility tracking. |
| extensions/kermit-coil3/api/android/kermit-coil3.api | Adds Android API dump for binary compatibility tracking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
faogustavo
left a comment
There was a problem hiding this comment.
Thanks for the PR. This looks good. Just adding some minor requests. Let us know if you can work on those.
| import coil3.util.Logger as Coil3Logger | ||
| import kotlin.jvm.JvmOverloads | ||
|
|
||
| class KermitCoil3Logger : Coil3Logger { |
There was a problem hiding this comment.
The constructors are a bit different from the other extensions. Can you standardize them please?
Maybe only these two are needed (Note that logger is not a val here, considering that you also apply the change below):
class KermitCoil3Logger(logger: KermitLogger) : Coil3Logger {
constructor(config: LoggerConfig, tag: String = "") : this(KermitLogger(config, tag))
// ....
}There was a problem hiding this comment.
Coil3 already has its own logger tag, so tag might be ambiguous/confusing here — it wouldn’t be used as the tag directly. Should I add a tagPrefix parameter instead, to prepend it to Coil3’s tag?
Also, regarding the config type: should the primary constructor take LoggerConfig and fall back to creating a new logger with a mutable config via toMutableConfig() if it isn't already mutable (similar to what we discussed above), or would you prefer typing it as MutableLoggerConfig directly to make the requirement explicit at the API level?
There was a problem hiding this comment.
OK, I was thinking about it, and it might make sense to invert my suggestion so that the default constructor receives the LoggerConfig and the tag, and the secondary one is the one "destructing" the logger to create a new one.
The reason is that we might not want to mutate the existing configuration/logger instance.
We also need to check the recommendation above about toMutableConfig and probably always create a new version for safety. So we always create a new instance that can be mutated.
Add `LoggerConfig.toMutableConfig()` to kermit-core's MutableLoggerConfig.kt Create a new logger when the provided LoggerConfig is immutable Remove some constructors
faogustavo
left a comment
There was a problem hiding this comment.
LGTM. @KevinSchildhorn can we have a second pair of eyes here? :D
| private val logger: KermitLogger = when (logger.config) { | ||
| is MutableLoggerConfig -> logger | ||
| else -> KermitLogger(config = logger.config.toMutableConfig()) | ||
| } |
There was a problem hiding this comment.
Let's always create a new logger to avoid mutating the old one.
| private val logger: KermitLogger = when (logger.config) { | |
| is MutableLoggerConfig -> logger | |
| else -> KermitLogger(config = logger.config.toMutableConfig()) | |
| } | |
| private val logger: KermitLogger = KermitLogger(config = logger.config.toMutableConfig()) |
Uh oh!
There was an error while loading. Please reload this page.