Skip to content
This repository has been archived by the owner on Feb 11, 2022. It is now read-only.

[PT-495] Support android kotlin projects #48

Merged

Conversation

tobiasheine
Copy link
Contributor

PT-495

Description

Scope of this PR is to add to make sure the detekt integration works also with android kotlin projects. Turned out that this works out of the box, so this PR just modifies the tests to consider also android kotlin projects.

Tests

  • created TestAndroidKotlinProject
  • changed DetektIntegrationTest to run all existing tests also against a TestAndroidKotlinProject

@@ -0,0 +1,78 @@
package com.novoda.test
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is basically a copy of TestAndroidProject. Changes are marked.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is not too important but with the comments I put below, it is possible that these can be 1 single class with different plugin configuration.

Then you can have TestKotlinProject.forAndroid() to configure the android plugins.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried that first but failed (see my comment below). Any help is appreciated since I also would like to get rid of this duplication.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just merged it. On a second thought, I don't think it is worth. Because that way, it will be more complicated.

}
dependencies {
classpath 'com.android.tools.build:gradle:3.0.1'
classpath 'org.jetbrains.kotlin:kotlin-gradle-plugin:1.2.20'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been added

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you may have left two whitespaces at the end of the line before when adding it:

image

EDIT: actually, there seem to be a few :) Nothing serious. Just my nitpicky eye 💃

Copy link
Contributor

Choose a reason for hiding this comment

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

👁‍🗨

jcenter()
}
apply plugin: 'com.android.library'
apply plugin: 'kotlin-android'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been added

Copy link
Contributor

Choose a reason for hiding this comment

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

to be honest you can also use the plugins and your withPlugin method to add these dynamically from test code.
The plugin id is org.jetbrains.kotlin.android
I think also com.android.library works too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I actually tried that for the kotlin plugin, but it didn't work. I got this error Extension with name 'android' does not exist.
And afaik this is because of the order. com.android.library needs to be applied first and I couldn't find that one here.

.collect { Map.Entry<String, List<String>> entry ->
"""$entry.key {
manifest.srcFile '${Fixtures.ANDROID_MANIFEST}'
kotlin {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has been changed

Copy link
Contributor

Choose a reason for hiding this comment

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

well, if you add your sources to java folder, you don't need this in test setup.

@rock3r rock3r self-requested a review January 26, 2018 09:24
Copy link
Contributor

@rock3r rock3r left a comment

Choose a reason for hiding this comment

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

LGTM 👍

}
dependencies {
classpath 'com.android.tools.build:gradle:3.0.1'
classpath 'org.jetbrains.kotlin:kotlin-gradle-plugin:1.2.20'
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you may have left two whitespaces at the end of the line before when adding it:

image

EDIT: actually, there seem to be a few :) Nothing serious. Just my nitpicky eye 💃

Copy link
Contributor

@tasomaniac tasomaniac left a comment

Choose a reason for hiding this comment

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

Will approve too.

It is up to @tobiasheine to merge or address my comments.

.entrySet()
.collect { Map.Entry<String, List<String>> entry ->
"""$entry.key {
manifest.srcFile '${Fixtures.ANDROID_MANIFEST}'
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe use double quotes just so there's more consistency? you can also just use $x.y here i believe

Copy link
Contributor

Choose a reason for hiding this comment

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

This is printing the entire config. In the generated one we want single quotes if possible.

@tasomaniac tasomaniac merged commit 3eed973 into PT-491/integrate_detekt Jan 26, 2018
@tasomaniac tasomaniac deleted the PT-495/support_android_kotlin_projects branch January 26, 2018 10:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants