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

Prepare compilation pipeline that hides API overloads - KProperty and ColumnAccessor, for compiler plugin workflow #959

Merged
merged 2 commits into from
Dec 16, 2024

Conversation

koperagen
Copy link
Collaborator

@koperagen koperagen commented Nov 20, 2024

With DataFrame public API now, for most operations there're 4 overloads.
KProperty and ColumnAccessor overloads were needed to support more convenient, robust safe access to columns. With compiler plugin, there's an idea these overloads won't be useful and only clutter code completion popup and make on-boarding harder.
This PR adds new tiny compiler plugin to compile the same sources where all these overloads are public, and make them internal in the process.
Publication of a new artifact will be in a separate PR because it's trickier that i thought

experimental:
image

core:
image

@koperagen koperagen self-assigned this Nov 20, 2024
@koperagen koperagen requested review from zaleslaw and Jolanrensen and removed request for zaleslaw November 20, 2024 18:06
@koperagen koperagen added this to the 0.16.0 milestone Nov 20, 2024
@koperagen koperagen changed the title Create new experimental artifact that hides API overloads KProperty and ColumnAccessor for compiler plugin workflow Create new experimental artifact that hides API overloads - KProperty and ColumnAccessor, for compiler plugin workflow Nov 20, 2024
@Jolanrensen
Copy link
Collaborator

Maybe the name -experimental could be a bit more descriptive. It's probably temporary, but still, we might want to publish another experiment in the future and name that dataframe-experimental. So I'd go with something like dataframe-core-minimal, or dataframe-core-for-compiler-plugin, or something like that.

Copy link
Collaborator

@Jolanrensen Jolanrensen left a comment

Choose a reason for hiding this comment

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

This would have taken a lot of time, wouldn't it? haha. Nice job thinking about the user experience in combination with the compiler plugin :)

I just have some suggestions regarding naming to prevent future headaches :)

(the module name is perfect btw, very descriptive :) )

plugins/public-api-modifier/build.gradle.kts Outdated Show resolved Hide resolved
}

val experimentalJarSources by tasks.registering(Jar::class) {
dependsOn("kspKotlin")
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's dependent on the generated sources, it should probably also depend on the task processKDocsMain, right?

Copy link
Collaborator Author

@koperagen koperagen Nov 22, 2024

Choose a reason for hiding this comment

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

I'm not sure, it seems to be modified by jar substitution rn, but i probably re-check and make an explicit dependency

publication {
publicationName = "dataframe-experimental"
description = "Excel support for Kotlin Dataframe"
composeOfTaskOutputs(listOf(experimentalJar, experimentalJarSources))
Copy link
Collaborator

Choose a reason for hiding this comment

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

omg, I'm gonna have to look into doing this for the publishing of generated sources too. That's way easier than temporarily modifying the kotlin main sourceset before publishing each time.

import org.jetbrains.kotlin.name.FqName

@OptIn(ExperimentalCompilerApi::class)
class PublicApiModifierRegistrar : CompilerPluginRegistrar() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is a good template compiler plugin haha. I don't think many are as easy as this one :)

@koperagen
Copy link
Collaborator Author

I was thinking dataframe-core-compact-api or just dataframe-core-compact. ExtendedApi annotation seems like an option too, but it kind of swaps default, right? Like, this way we need dataframe-core by default and dataframe-core-extended :)

@Jolanrensen
Copy link
Collaborator

I was thinking dataframe-core-compact-api or just dataframe-core-compact.

Maybe... Although "compact" suggests everything is still included, just more compressed somehow, which is not the case, we actually take stuff out.

AI suggestions:

  • dataframe-core-lite (good option!)
  • dataframe-core-core (LOL no XD)
  • dataframe-core-basic (meh)
  • dataframe-core-mini (maybe)
  • dataframe-core-reduced (too long imo)
  • dataframe-core-slim (I guess...)
  • dataframe-core-minimal (could be a good option)

ExtendedApi annotation seems like an option too, but it kind of swaps default, right? Like, this way we need dataframe-core by default and dataframe-core-extended :)

Yeah I see what you mean.. Anyway, we can always rename the annotation later :) @FullApi could be an option?

AI suggestions:

  • @NotInLiteVersion (lol)
  • @FullCoreOnly (mmm)
  • @ExcludedFromLite (is clear tho)

@koperagen koperagen force-pushed the df-api branch 2 times, most recently from 7ed1f1f to 1100d90 Compare December 16, 2024 12:40
@koperagen koperagen changed the title Create new experimental artifact that hides API overloads - KProperty and ColumnAccessor, for compiler plugin workflow Prepare compilation pipeline that hides API overloads - KProperty and ColumnAccessor, for compiler plugin workflow Dec 16, 2024
@koperagen
Copy link
Collaborator Author

koperagen commented Dec 16, 2024

@Jolanrensen I decided to split this PR. I'll setup publishing later - it's trickier than i thought :(

@koperagen koperagen merged commit bfcee2c into master Dec 16, 2024
6 checks passed
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