Skip to content

Add coil3 extension - #479

Open
lightsummer233 wants to merge 6 commits into
touchlab:mainfrom
lightsummer233:main
Open

Add coil3 extension#479
lightsummer233 wants to merge 6 commits into
touchlab:mainfrom
lightsummer233:main

Conversation

@lightsummer233

@lightsummer233 lightsummer233 commented Jul 19, 2026

Copy link
Copy Markdown
截屏2026-07-31 16 06 46

Copilot AI review requested due to automatic review settings July 19, 2026 09:59

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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-coil3 Gradle module and add Coil 3 to the version catalog.
  • Introduce KermitCoil3Logger implementing Coil 3’s Logger, 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.

Comment thread extensions/kermit-coil3/README.md
Comment thread extensions/kermit-coil3/README.md
Comment thread extensions/kermit-coil3/build.gradle.kts Outdated
Comment thread extensions/kermit-coil3/build.gradle.kts Outdated
@lightsummer233
lightsummer233 marked this pull request as draft July 19, 2026 10:22
@lightsummer233
lightsummer233 marked this pull request as ready for review July 31, 2026 08:03
@faogustavo faogustavo assigned faogustavo and unassigned faogustavo Aug 11, 2026
@faogustavo
faogustavo self-requested a review August 11, 2026 14:21

@faogustavo faogustavo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the PR. This looks good. Just adding some minor requests. Let us know if you can work on those.

Severity.Warn -> Coil3Logger.Level.Warn
Severity.Error -> Coil3Logger.Level.Error
Severity.Assert -> Coil3Logger.Level.Error // Map Assert to Error for Coil3Logger
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can you make those internal, please? I don't thik that we need to export them

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Sure, I will finish it tomorrow.

Comment on lines +61 to +66
override var minLevel: Coil3Logger.Level
get() = logger.config.minSeverity.toCoil3LoggerLevel()
set(value) {
logger.mutableConfig.minSeverity = value.toKermitSeverity()
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This could crash if the state is not mutable. We may need to change the logger config instance when we create this type like this:

    private val logger: KermitLogger =
        when (logger.config) {
            is MutableLoggerConfig -> logger
            else -> KermitLogger(config = logger.config.toMutableConfig(), tag = logger.tag)
        }

This toMutableConfig is something like this (suggested by Claude):

/**
 * Converts a LoggerConfig to a MutableLoggerConfig.
 * @returns itself if it is mutable, a new instance of [MutableLoggerConfig] inheriting original configuration otherwise
 */
fun LoggerConfig.toMutableConfig(): MutableLoggerConfig {
    if (this is MutableLoggerConfig) return this
    return mutableLoggerConfigInit(logWriters = logWriterList.toTypedArray(), minSeverity = minSeverity)
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Should I add LoggerConfig.toMutableConfig() as a public extension in kermit-core’s MutableLoggerConfig.kt? It seems like a generally useful utility beyond just this extension.

import coil3.util.Logger as Coil3Logger
import kotlin.jvm.JvmOverloads

class KermitCoil3Logger : Coil3Logger {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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))
    // ....
}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Should tag be renamed to tagPrefix here? Since Coil3 already has its own logger tag concept, tag might be ambiguous/confusing in this context.

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants