Skip to content

allow applying ksp to custom configs #842

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

Merged
merged 4 commits into from
Sep 3, 2024

Conversation

mgroth0
Copy link
Contributor

@mgroth0 mgroth0 commented Aug 26, 2024

Fixes #841

As described in the issue, I needed to modify the ConvenienceSchemeGeneratorPlugin again (this is sort of part 2 after #647). This PR is basically what I've been using for myself all of this time so I could correctly use dataframes in my custom source set.

I think my solution is hacky and I am not sure this PR should be accepted, but it does get the job done for me.

I allow setting custom configs as either a gradle property or an extra property. The reason why is because while in my own code I set the extra property programatically based on some custom build logic that detects if a module has both android and jvm source sets, the extra property can only be set programatically wheras the regular property can be set in a gradle.properties file. I only use the extra property, but I expect if this PR is merged that others might want to do it via gradle.properties. gradle.properties would not work for me, however, because I need to set it programatically which is only supported by extra properties. Therefore, unforunately, the best I could think of was allow both.

@Jolanrensen Jolanrensen self-assigned this Aug 27, 2024
@Jolanrensen
Copy link
Collaborator

I think it's a good idea. The method we used was already hacky at the time of writing #647, at least something like this could solve the case where the ksp config has a non-standard name.

I would try to improve the logic a bit. Does target.findProperty() work to find both properties and extraProperties?

We also probably should make "kotlin.dataframe.add.ksp" and "kotlin.dataframe.ksp.configs" constant with some documentation at the top of the file.

Finally, don't forget to run ktlintFormat/ktlintCheck :)

- use findProperty, no longer warn since gradle has a conventional precendence for regular vs extra property
- put properties as  in companion
- write some documentation on the properties
- ran
@mgroth0
Copy link
Contributor Author

mgroth0 commented Aug 30, 2024

I didn't know about findProperty, nice catch!

@Jolanrensen Jolanrensen self-requested a review September 3, 2024 09:58
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.

Just one small suggestion, the rest looks good, thank you very much!

target.logger.warn(
"Configuration '$testKspCfg' not found. Please make sure the KSP plugin is applied.",
)
val overriddenConfigs = target.findProperty(PROP_KSP_CONFIGS)?.let { (it as String) }?.split(",")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we should also trim the entries in the list. When a user writes: "config1, config2", we now get the configs "config1" and " config2". I'm not sure getByName can find it like that

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll add it quickly so we can merge :)

@Jolanrensen Jolanrensen merged commit ea7dbe8 into Kotlin:master Sep 3, 2024
4 checks passed
@mgroth0
Copy link
Contributor Author

mgroth0 commented Sep 3, 2024

Thanks for the help and for accepting :)

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.

KSP generated code doesn't work with custom source sets
2 participants