-
Notifications
You must be signed in to change notification settings - Fork 72
Splitting off Jupyter yet again #1095
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
…t some codegen logic. Refactored codegen paths from org.jetbrains.dataframe -> org.jetbrains.kotlinx.dataframe
69b58a6
to
4cfa0ce
Compare
ae058e5
to
7ae67ff
Compare
41fba75
to
78951d1
Compare
# Conflicts: # plugins/kotlin-dataframe/build.gradle.kts
@@ -28,3 +25,6 @@ public data class CodeWithConverter(val declarations: Code, val converter: (Vari | |||
else -> declarations + "\n" + converter(name) | |||
} | |||
} | |||
|
|||
public typealias Code = String |
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.
Is it an option to not have these in public api?
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 think they're part of our codegen api. CodeWithConverter is public too after all
@@ -26,7 +26,8 @@ repositories { | |||
fun ExternalModuleDependency.excludeJaiCore() = exclude("javax.media", "jai_core") | |||
|
|||
dependencies { | |||
api(project(":core")) | |||
api(projects.core) | |||
implementation(projects.dataframeJupyter) |
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.
Why do we need geo to depend on impl jupyter?
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.
It uses useSchema<>()
in its jupyter integration
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.
If we expect to use geo Integration to work only when jupyter Integration is configured (which should be), dependency can be compileOnly
here
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'm not 100% sure, @AndreiKingsley ?
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.
Let's fix this after merging, so we can revert back to java 8 sooner
Fixes #775
:dataframe-jupyter
org.jetbrains.dataframe
->org.jetbrains.kotlinx.dataframe
in codegen partsTrying out Restrikt to do this responsibly.Restrikt seems too unstable for DF. Often getting compile errors in FIR or NoSuchElementExceptions in tests.