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

feat: add kotlin support #896

Merged
merged 19 commits into from
Jan 27, 2020
Merged

feat: add kotlin support #896

merged 19 commits into from
Jan 27, 2020

Conversation

erisu
Copy link
Member

@erisu erisu commented Jan 16, 2020

Motivation and Context

To finish off the change requests and close out PR #441

closes: #441

Description

What this PR does:

  • Cherry-picks commit changes from CB-14089: (android) Add Kotlin support #441
  • Rename preference option EnableKotlin to GradlePluginKotlinEnabled (Default: false)
    • Since the preference has not been released or used in any other repos, the name change is safe. This change is only an update from the original PR's proposal.
  • Added preference options GradlePluginKotlinCodeStyle (Default: official)
  • Added preference option GradlePluginKotlinVersion (Default: 1.3.50)
  • Apply kotlin-android-extensions plugin as well when GradlePluginKotlinEnabled is true
  • Changed compile to implementation

Testing

  • npm t

Checklist

  • I've run the tests to see all new and existing tests pass
  • If this Pull Request resolves an issue, I linked to the issue in the text above (and used the correct keyword to close issues using keywords)
  • I've updated the documentation if necessary (Pending Work for Cordova Docs Repo)

@erisu erisu added this to the 9.0.0 milestone Jan 16, 2020
@erisu erisu requested review from dpogue and timbru31 January 16, 2020 05:10
@erisu erisu changed the title dFeat/kotlin support feat: add kotlin support Jan 16, 2020
@erisu erisu mentioned this pull request Jan 16, 2020
3 tasks
@codecov-io
Copy link

codecov-io commented Jan 16, 2020

Codecov Report

Merging #896 into master will decrease coverage by 0.14%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #896      +/-   ##
==========================================
- Coverage   66.41%   66.27%   -0.15%     
==========================================
  Files          19       19              
  Lines        1849     1853       +4     
==========================================
  Hits         1228     1228              
- Misses        621      625       +4
Impacted Files Coverage Δ
bin/templates/cordova/lib/prepare.js 34.65% <0%> (-0.47%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d01ed80...99ee5d0. Read the comment docs.

@erisu
Copy link
Member Author

erisu commented Jan 16, 2020

@timbru31

I went ahead and created a new Kotlin support PR so we can quickly apply all the changes and improvements to get Kotlin prepared and released hopefully in the next day or 2.

Per your change request in the previous PR, I have:

  • Updated Kotlin to the latest version

  • Replaced mavenCentral with jcenter in the test/app/build.gradle

NOTICE:
This change is not applied to the actual build.gradle that is used when building a Cordova project. The actual file, bin/templates/project/app/build.gradle, still uses in the following order:

mavenCentral -> google -> jcenter

If these are to be updated, it should be taken care of in a separate PR

  • Per your comment about kotlin:kotlin-stdlib-jdk7 I believe it is best to keep it as kotlin:kotlin-stdlib-jdk7. A newly created Android project with the official Android Studio uses kotlin:kotlin-stdlib-jdk7. Probably best to not do something different.

@erisu
Copy link
Member Author

erisu commented Jan 16, 2020

@timbru31 I just added the Kotlin source set as well, per your change request's overview comment. The only thing I was not sure is if it was necessary to wrap that behind a flag too.

@dpogue
Copy link
Member

dpogue commented Jan 16, 2020

@timbru31 I just added the Kotlin source set as well, per your change request's overview comment. The only thing I was not sure is if it was necessary to wrap that behind a flag too.

IMO no need for a flag for adding the src/main/kotlin folder. Note that currently all plugin source files will end up in src/main/java because there's no special handling to detect kotlin files.

@erisu erisu force-pushed the feat/kotlin-support branch 2 times, most recently from c66388c to 7e5443d Compare January 18, 2020 05:46
@@ -19,16 +19,16 @@
// Top-level build file where you can add configuration options common to all sub-projects/modules.

buildscript {
ext.kotlin_version = '1.3.50'
Copy link
Member

Choose a reason for hiding this comment

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

is this the Android Studio default?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, if you create a blank "No Activity" project in Android Studio, it has in the project's build.gradle:

ext.kotlin_version = '1.3.50'

Copy link

@knight9999 knight9999 left a comment

Choose a reason for hiding this comment

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

It looks fine.
I can run Kotlin code successfully with this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants