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

Support convert shortcuts #1118

Merged
merged 2 commits into from
Apr 9, 2025
Merged

Support convert shortcuts #1118

merged 2 commits into from
Apr 9, 2025

Conversation

koperagen
Copy link
Collaborator

I tried to minimize repetition and come up with somewhat generic solution. However the way framework works, we still must have delegated properties matching function arguments. I decided to not provide a way for interpreter to ignore any unknown arguments in this PR

@koperagen koperagen added the Compiler plugin Anything related to the DataFrame Compiler Plugin label Apr 4, 2025
@koperagen koperagen added this to the 1.0.0-Beta1 (0.16) milestone Apr 4, 2025
@koperagen koperagen requested a review from zaleslaw April 4, 2025 11:45
@koperagen koperagen self-assigned this Apr 4, 2025
public fun <T, R : URL?> Convert<T, R>.toIFrame(
@JvmName("toIframeFromUrlNullable")
@Refine
@Converter<IFRAME>(IFRAME::class, nullable = true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The compiler can infer the <> part if you pass a KClass right? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Surprisingly, no. If i remove it, compiler complains about wrong number of type arguments

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's interesting... Then again, I've never used an annotation that takes a type argument before, maybe it's a limitation

Copy link
Collaborator

Choose a reason for hiding this comment

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

but do you use the type argument? Or could it work if you accept KClass<*>?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Type argument is not needed. Initially i wanted to only use type argument, but compiler doesn't see it - only KClass get class expression. Get class works as expected without type argument

import org.jetbrains.kotlinx.dataframe.io.*
import kotlinx.datetime.*

fun box(): String {
Copy link
Collaborator

Choose a reason for hiding this comment

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

woow cool :D

@koperagen koperagen force-pushed the convert-shortcuts branch from d64ef01 to 1d73a6d Compare April 4, 2025 12:58

override fun Arguments.interpret(): PluginDataFrameSchema {
val converterAnnotation = functionCall.calleeReference.toResolvedFunctionSymbol()?.getAnnotationByClassId(Names.CONVERTER_ANNOTATION, session)
val to = converterAnnotation?.getKClassArgument(Name.identifier("klass"), session)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this an only place where we could extract info from annotations?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

for now it's the first interpreter that relies on annotations - to reduce number of classes. imagine 20 interpreters for each to* function instead of these 3

@@ -402,17 +482,51 @@ public fun DataColumn<String?>.convertToLocalTime(
return map { it?.let { converter(it.trim()) ?: error("Can't convert `$it` to LocalTime") } }
}

@JvmName("toLocalTimeFromTLongNullable")
@Refine
@Converter(LocalTime::class, nullable = true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's kind of pattern to define type info for outputs, isn't it? a-la @ReturnType as a more common approach

@koperagen koperagen merged commit cce682b into master Apr 9, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Compiler plugin Anything related to the DataFrame Compiler Plugin
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants