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.

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

Add KPlatform & KDefaultPlatform #12

Merged
merged 18 commits into from
Aug 8, 2023
Merged

Add KPlatform & KDefaultPlatform #12

merged 18 commits into from
Aug 8, 2023

Conversation

buenaflor
Copy link
Owner

@buenaflor buenaflor commented Aug 2, 2023

I came across two issues in this PR:

Issue 1

findLoggingClass uses Class which is not easily typealiased in commonMain because Class and KClass have different signatures:

  • public class Class
  • public interface KClass

My solution here is to create an expect class Klass<T: Any> which will typealias directly to Class on JVM and on iOS it will save the KClass reference in a variable.

  • JVM: public actual typealias Klass<T> = Class<T>
  • iOS: public actual class Klass<T : Any>(public val kClass: KClass<T>)

Additionally KClass will have an extension function: public expect fun <T : Any> KClass<T>.toKlass()

The usage looks like this in commonMain

KPlatform.getCallerFinder().findLoggingClass(MyClass::class.toKlass())

Note: I couldn't come up with a better name other than Klass but I'm pretty sure there is a much better name for this

Why this solution

It's important to be able to use actual typealias on the JVM side so I opted for keeping the Class signature even though it's never going to be used directly as you can only create a Klass through the toKlass() method through KClass in the common code.

Issue 2

If the findLoggingClass is called from a companion object in Kotlin the StackFrame classname will include an extra $Companion - this doesn't work in the default Java implementation because this case is not accounted for (probably because the codebase is written only in Java) and will throw an IllegalStateException because it cannot find the correct StackFrame.

Example:

Kotlin: com.example.test.FluentLogger$Companion
Java: com.example.test.FluentLogger

Solution:

// KFlogger: To handle Kotlin companion calls we need to check for names suffixed with $Companion
return f -> f.getClassName().equals(name) || f.getClassName().equals(name + "$Companion");

However this solution requires a small change in the Flogger codebase.

@MarkCMann

@buenaflor buenaflor marked this pull request as ready for review August 3, 2023 20:38
@buenaflor buenaflor marked this pull request as draft August 3, 2023 20:40
@buenaflor buenaflor marked this pull request as ready for review August 4, 2023 01:08
@@ -61,7 +61,8 @@ public StackTraceElement[] getStackForCaller(Class<?> target, int maxDepth, int

private Predicate<StackFrame> isTargetClass(Class<?> target) {
String name = target.getName();
return f -> f.getClassName().equals(name);
// KFlogger: To handle Kotlin companion calls we need to check for names suffixed with $Companion
return f -> f.getClassName().equals(name) || f.getClassName().equals(name + "$Companion");
Copy link
Owner Author

Choose a reason for hiding this comment

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

Small change in the Java Flogger codebase

Comment on lines +5 to +9
public actual typealias Klass<T> = Class<T>

public actual fun <T : Any> KClass<T>.toKlass(): Klass<T> {
return this.java
}
Copy link
Owner Author

Choose a reason for hiding this comment

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

Klass workaround to make actual typealias work with Platform

Choose a reason for hiding this comment

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

Looks good, reflection is one of the rough edges for KMP

Comment on lines +5 to +9
public actual typealias Klass<T> = Class<T>

public actual fun <T : Any> KClass<T>.toKlass(): Klass<T> {
return this.java
}

Choose a reason for hiding this comment

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

Looks good, reflection is one of the rough edges for KMP


import kotlin.reflect.KClass

public expect class Klass<T : Any>

Choose a reason for hiding this comment

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

Do you want to define some properties, functions, or extension functions here? It's unclear what benefits you get by making this class an expected class -- although it looks like the usages are opaque to callers outside KFlogger so maybe this is okay.

Copy link
Owner Author

@buenaflor buenaflor Aug 8, 2023

Choose a reason for hiding this comment

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

Usages won't be really a concern outside of KFlogger, the reason why it's an expected class is because it typealiases directly on JVM to Class and on iOS it saves the reference to the KClass. It's basically a "bridge" class without functionality just so we can keep the Java typealias. In it's current state it's only used in findLoggingClass

@buenaflor buenaflor merged commit ac0e436 into main Aug 8, 2023
3 checks passed
@buenaflor buenaflor deleted the feat/defaultplatform branch August 8, 2023 01:09
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.

2 participants