-
Notifications
You must be signed in to change notification settings - Fork 72
Configure writerside to include tables #324
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
Conversation
8ace975
to
61a56ab
Compare
95dade0
to
e04e0cd
Compare
467cff3
to
d1e4f88
Compare
d6ff368
to
ccc19e3
Compare
ccc19e3
to
014fa67
Compare
c92dc2b
to
0fd4d4f
Compare
I'll add documentation later as the process of adding tables to documentation should be refined. But the general idea is that you annotate a function and the compiler plugin adds "also { }" call after each "interesting" expression. That enables us to process samples (compose HTML outputs) without manually rewriting all samples using |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! Makes it easy to see intermediate dataframe states in documentation. However, we do need to consider the extra maintainability of another compiler plugin. Especially when we need to move to k2 eventually. It's difficult to understand most of the testing of the plugin as well as the code itself. But that's the case with most compiler plugins. That said, I do think it's worth it :)
@@ -45,6 +46,7 @@ df.convert { name.firstName and name.lastName }.to { it.length() } | |||
df.convert { weight }.toFloat() | |||
``` | |||
|
|||
<dataFrame src="org.jetbrains.kotlinx.dataframe.samples.api.Modify.convertTo.html"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which of the convert functions is rendered here? or all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/explainer/PluginCallback.kt
Outdated
Show resolved
Hide resolved
core/src/test/kotlin/org/jetbrains/kotlinx/dataframe/explainer/PluginCallback.kt
Outdated
Show resolved
Hide resolved
0 -> { | ||
val table = convertToHTML(expression.df) | ||
val description = table.copy( | ||
body = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here with @language
plugins/expressions-converter/src/org/jetbrains/kotlinx/dataframe/ExplainerIrTransformer.kt
Outdated
Show resolved
Hide resolved
this is a verbose variant of manual generation, should be improved at some point
5838988
to
0485832
Compare
settings.gradle.kts
Outdated
@@ -32,3 +33,4 @@ pluginManagement { | |||
} | |||
} | |||
|
|||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
another newline? haha
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
return data | ||
} | ||
|
||
var action: PluginCallback = PluginCallback { source, name, df, id, receiverId, containingClassFqName, containingFunName, statementIndex -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better :D
} | ||
} | ||
val alsoLambdaExpression = IrFunctionExpressionImpl( | ||
-1, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missed some named arguments. A good linter could probably help with these... We really need to fix that soon :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's for a linter to decide :D I would add them, but generally, in my opinion, it's up for a debate at review
type = pluginContext.irBuiltIns.functionN(2) | ||
.typeWith(listOf(expression.type, pluginContext.irBuiltIns.unitType)), | ||
alsoLambda, | ||
IrStatementOrigin.LAMBDA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same with trailing comma's. I think currently I'm the one adding them and you're the one leaving them out haha.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this constructor is quite a stable API and won't require changing parameters order :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You never know, especially with k2 around the corner. We had those incompatibility issues with korro because in 1.8.20 they suddenly added another parameter. Naming them at least helps a bit :) (Also for reviewing)
@Jolanrensen Thank you! |
We now have a custom component for embedding tables. This feature is available on a custom branch of writerside, but still should reach our CI/CD before KotlinConf #233

Still need to figure out some details like displaying before / after, schema and maybe giving some visual clues to highlight what the operation does.