Ktlint integration #110

tasomaniac · 2018-07-07T21:43:44Z

This PR adds Ktlint integration. Ktlint does not have a gradle plugin. There are 2 available gradle plugins and I chose org.jlleitschuh.gradle.ktlint since it handles Android flavors in a nice way.

Considerations

ignoreFailures = true because our plugin will fail if thresholds are passed
reporters are hardcoded to checkstyle and plain. Html is not supported 😞
Used the default CollectCheckstyleViolationsTask to parse the xml. The tool explicitly says that it uses checkstyle xml report so I think it is better than duplicating the class. If you want, I can duplicate.

Fixes #50

hal9002 · 2018-07-07T21:43:45Z

Can one of the admins review this PR?

tasomaniac · 2018-07-07T21:44:37Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/ktlint/KtlintConfigurator.groovy

+
+    private def createCollectViolationsTask(Violations violations, def sourceSetName) {
+        project.tasks.create("collectKtlint${sourceSetName.capitalize()}Violations", CollectCheckstyleViolationsTask) { task ->
+            task.xmlReportFile = new File(project.buildDir, "reports/ktlint/ktlint-${sourceSetName}.xml")


No way to change the output in the plugin. Hardcoded for now.

Does this mean we get ktlint-main.xml? Never used the underlying Ktlint plugin, so not sure :)

I think Gradle plugin provides the file name and path internally to ktlint tool.

It has report per sourceSet in pure Kotlin, and report per variant in Android projects.

The path and file names are hardcoded and not configurable by the user.

tasomaniac · 2018-07-07T21:45:18Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/CollectViolationsTask.groovy

 import org.gradle.api.tasks.TaskAction

 abstract class CollectViolationsTask extends DefaultTask {

+    @InputFile


Added this to make the task better cacheable. Don't think it will hurt in any way. WDYT?

tasomaniac · 2018-07-07T21:46:03Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/ktlint/KtlintConfigurator.groovy

+            config.delegate = ktlint
+            config()
+
+            project.afterEvaluate {


Just a notice: Had to add project.afterEvaluate since the plugin itself uses afterEvaluate too. Don't think it is a problem.

Should be ok, at least for now, since as you say that's what the plugin we're wrapping does

tasomaniac · 2018-07-07T21:47:41Z

plugin/src/test/groovy/com/novoda/test/TestAndroidKotlinProject.groovy

@@ -14,7 +14,8 @@ buildscript {
    }
 }
 plugins {
-    ${formatPlugins(project)}
+    ${formatPlugins(project)}   
+    id 'com.novoda.static-analysis'


It is better if the static-analysis plugin is added later. I needed to do this earlier. It may not be needed anymore. But I think it is a good change anyways since it makes it explicit. Before it was added by the method withPlugin in the constructor.

tasomaniac · 2018-07-07T21:48:19Z

plugin/src/test/groovy/com/novoda/test/TestAndroidKotlinProject.groovy

@@ -58,7 +59,7 @@ ${formatExtension(project)}
                .collect { Map.Entry<String, List<String>> entry ->
            """$entry.key {
            manifest.srcFile '${Fixtures.ANDROID_MANIFEST}'
-            kotlin {
+            java {


Android projects uses java sourceSet for mixed Kotlin and Java sources.

tasomaniac · 2018-07-07T21:48:59Z

plugin/src/test/groovy/com/novoda/test/TestKotlinProject.groovy

 }

 apply plugin: 'kotlin'
+
+repositories { 


Was needed for ktlint jar to be resolved in pure kotlin project.

rock3r · 2018-07-09T08:48:28Z

Trying to close and reopening to see if the CI picks it up

rock3r

Great job! Have a couple of questions — doubts really — then good for me

rock3r · 2018-07-09T08:53:12Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/ktlint/KtlintConfigurator.groovy

+            config.delegate = ktlint
+            config()
+
+            project.afterEvaluate {


Should be ok, at least for now, since as you say that's what the plugin we're wrapping does

rock3r · 2018-07-09T10:52:54Z

plugin/src/main/groovy/com/novoda/staticanalysis/internal/ktlint/KtlintConfigurator.groovy

+
+    private def createCollectViolationsTask(Violations violations, def sourceSetName) {
+        project.tasks.create("collectKtlint${sourceSetName.capitalize()}Violations", CollectCheckstyleViolationsTask) { task ->
+            task.xmlReportFile = new File(project.buildDir, "reports/ktlint/ktlint-${sourceSetName}.xml")


Does this mean we get ktlint-main.xml? Never used the underlying Ktlint plugin, so not sure :)

rock3r · 2018-07-09T10:53:08Z

plugin/src/test/fixtures/sources/ktlint/with-error/KtLintViolator.kt

+class KtLintViolator {
+
+    override fun equals(other: Any?): Boolean {
+        // this is not allowed


What is not allowed?

Haha I think always returning true is not allowed. This is just a fixture and a copy paste from detekt fixtures.

Here, I need any Kotlin code with style problem. Would be happy to change. 👍

rock3r

LGTM, maybe @mr-archano can put his laser 👀 on this too :)

tasomaniac · 2018-07-17T15:37:37Z

@mr-archano bump 👊

mr-archano · 2018-07-29T10:28:09Z

plugin/src/test/groovy/com/novoda/test/TestJavaProject.groovy

@@ -5,7 +5,8 @@ final class TestJavaProject extends TestProject<TestJavaProject> {
    private static final Closure<String> TEMPLATE = { TestProject project ->
        """
 plugins {
-    ${formatPlugins(project)}
+    ${formatPlugins(project)} 
+    id 'com.novoda.static-analysis'


why this change?

Explained here already. Can check if reverting is fine or not. Users are expected to apply static analysis at the last position. Also makes it more obvious here.
#110 (comment)

sorry I actually read it, but somehow missed the connection 🙈

mr-archano · 2018-07-29T10:28:37Z

plugin/src/test/groovy/com/novoda/test/TestKotlinProject.groovy

@@ -15,9 +15,15 @@ buildscript {

 plugins {
    ${formatPlugins(project)}
+    id 'com.novoda.static-analysis'


same as above.

mr-archano · 2018-07-31T15:46:55Z

Thanks @tasomaniac for this! A new snapshot should be available once merged to develop

Tapchicoma · 2018-09-05T15:33:20Z

reporters are hardcoded to checkstyle and plain. Html is not supported

Opened an issue: JLLeitschuh/ktlint-gradle#125

tasomaniac · 2018-09-07T08:15:15Z

Thanks @Tapchicoma

I intended to do that but then I forgot.

tasomaniac added 2 commits July 7, 2018 22:50

Initial ktlint integration with tests for Android projects.

ab3528e

Fix integration for pure kotlin projects

b280376

tasomaniac commented Jul 7, 2018

View reviewed changes

rock3r closed this Jul 9, 2018

rock3r reopened this Jul 9, 2018

rock3r self-requested a review July 9, 2018 08:50

rock3r reviewed Jul 9, 2018

View reviewed changes

Merge branch 'develop' into taso/ktlint-integration

66ee61a

rock3r approved these changes Jul 9, 2018

View reviewed changes

Merge branch 'develop' into taso/ktlint-integration

73f8d9d

mr-archano suggested changes Jul 29, 2018

View reviewed changes

mr-archano approved these changes Jul 31, 2018

View reviewed changes

mr-archano merged commit 67894b2 into novoda:develop Jul 31, 2018

tasomaniac deleted the taso/ktlint-integration branch August 1, 2018 18:51

mr-archano mentioned this pull request Sep 4, 2018

Add support for KtLint for Kotlin projects #50

Closed

mr-archano mentioned this pull request Oct 5, 2018

Multiple different ' ktlint' plugin references #135

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ktlint integration #110

Ktlint integration #110

tasomaniac commented Jul 7, 2018

hal9002 commented Jul 7, 2018

tasomaniac Jul 7, 2018

rock3r Jul 9, 2018

tasomaniac Jul 9, 2018

tasomaniac Jul 7, 2018

tasomaniac Jul 7, 2018

rock3r Jul 9, 2018

tasomaniac Jul 7, 2018

tasomaniac Jul 7, 2018

tasomaniac Jul 7, 2018

rock3r commented Jul 9, 2018

rock3r left a comment

rock3r Jul 9, 2018

rock3r Jul 9, 2018

rock3r Jul 9, 2018

tasomaniac Jul 9, 2018

tasomaniac Jul 9, 2018

rock3r left a comment

tasomaniac commented Jul 17, 2018

mr-archano Jul 29, 2018

tasomaniac Jul 31, 2018

mr-archano Jul 31, 2018

mr-archano Jul 29, 2018

mr-archano commented Jul 31, 2018

Tapchicoma commented Sep 5, 2018

tasomaniac commented Sep 7, 2018

Ktlint integration #110

Ktlint integration #110

Conversation

tasomaniac commented Jul 7, 2018

Considerations

hal9002 commented Jul 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rock3r commented Jul 9, 2018

rock3r left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rock3r left a comment

Choose a reason for hiding this comment

tasomaniac commented Jul 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mr-archano commented Jul 31, 2018

Tapchicoma commented Sep 5, 2018

tasomaniac commented Sep 7, 2018