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

Improvements #83

Open
wants to merge 31 commits into
base: master
Choose a base branch
from
Open

Improvements #83

wants to merge 31 commits into from

Conversation

jasonsparc
Copy link

Major changes:

  • Android & JVM support – see, Support other Kotlin plugins #40

  • Made it easier to support more Kotlin plugins.

  • No longer filters out non-test compilations whose name ends with *test, such as latest, fastest, cutest, shortest, etc.

    • Note: This is a behavior change. It is thus a breaking change. Even so, the change is really just a single tiny commit. See commit, a19327a

Minor changes:

  • Bumped Gradle to 8.0.2 (from 8.0.1)

  • Some error & warning suppressions.

  • Some code reorganizations.


Remarks:

  • I may submit another PR after this, in order to add support for the Kotlin/JS plugin (which I believe, might be a small change compared to this PR).

  • I'll leave it up to you to give an appropriate version for the Kotlin Gradle Plugin (as I decided not to change it).

    • Right now, your plugin is targeting Kotlin 1.8.X; however, I believe that lowering the version to support at least 1.7.X would've been better, as there are a lot of plugins out there that targets that version. But then again, this is just my recommendation; I'll leave this up to you to consider.

    • Also, the Android Gradle Plugin is only used in tests and samples, and thus, I didn't bother touching it. It's okay for its version to be too high (so long as it's not too high that the build would warn you about the Kotlin Gradle Plugin's version being too low).

    • The README would need to be updated to reflect the new Kotlin version requirements.

  • This PR might be a bit big. It may be easier to visualize the changes by using the IDE's diffing features (or at least use any other tool instead of relying on GitHub), as some changes are either indentation changes or moving files.

  • There are now 8 sample projects (4 KTS + 4 Groovy), instead of just 2. Maintaining them might be a hassle without IDE refactoring support. I'll leave it up to you on how to best organize them to have the IDE support integrated.

    • Perhaps you should integrate them with the main project, and if you do, be warned that you might have to change some rootProject.name properties in both of the 'sample' projects' settings file, as both are separate included builds.

    • My recommendation is to just merge the two 'sample' included builds into a single included build. I don't know how best to do that as you might have your own preferences; thus, I'll leave this up to you.

  • All these are just my recommendation. Feel free to disregard them (or some).

…ch as `latest`, `fastest`, `cutest`, `shortest`, etc.
@esafak
Copy link

esafak commented Jul 5, 2023

@yshrsmz are you going to be able to review this, or should @jasonsparc split it up?

@yshrsmz
Copy link
Owner

yshrsmz commented Jul 6, 2023

I'm super busy these days, so can't say anything about ETA but wanting to review this. Sorry

@yshrsmz
Copy link
Owner

yshrsmz commented Jul 12, 2023

Sorry for being late.

Right now, your plugin is targeting Kotlin 1.8.X; however, I believe that lowering the version to support at least 1.7.X would've been better, as there are a lot of plugins out there that targets that version. But then again, this is just my recommendation; I'll leave this up to you to consider.

We need to check the compatibility with the new Android source set layout introduced in Kotlin 1.8.0.
And 1.7.x is fine if BuildKonfig can handle the new spec.

https://kotlinlang.org/docs/multiplatform-android-layout.html#check-the-compatibility

Android Gradle Plugin

Our primary focus is Kotlin Multiplatform, so I'm leaning towards downgrading AGP to 7.x, if AGP 8.x can handle plugins developed with AGP 7.x.

KGP/AGP/Gradle compatibility table: https://kotlinlang.org/docs/gradle-configure-project.html#apply-the-plugin

edit: yeah, we should downgrade AGP according to the table

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.

3 participants