-
Notifications
You must be signed in to change notification settings - Fork 27
Ktlint integration #110
Ktlint integration #110
Changes from 2 commits
ab3528e
b280376
66ee61a
73f8d9d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
package com.novoda.staticanalysis.internal.ktlint | ||
|
||
import com.novoda.staticanalysis.StaticAnalysisExtension | ||
import com.novoda.staticanalysis.Violations | ||
import com.novoda.staticanalysis.internal.Configurator | ||
import com.novoda.staticanalysis.internal.VariantFilter | ||
import com.novoda.staticanalysis.internal.checkstyle.CollectCheckstyleViolationsTask | ||
import org.gradle.api.GradleException | ||
import org.gradle.api.NamedDomainObjectContainer | ||
import org.gradle.api.Project | ||
import org.gradle.api.Task | ||
|
||
class KtlintConfigurator implements Configurator { | ||
|
||
private static final String KTLINT_PLUGIN = 'org.jlleitschuh.gradle.ktlint' | ||
private static final String KTLINT_NOT_APPLIED = 'The Ktlint plugin is configured but not applied. Please apply the plugin in your build script.\nFor more information see https://github.com/jeremymailen/kotlinter-gradle' | ||
|
||
private final Project project | ||
private final Violations violations | ||
private final Task evaluateViolations | ||
private final VariantFilter variantFilter | ||
|
||
static KtlintConfigurator create(Project project, | ||
NamedDomainObjectContainer<Violations> violationsContainer, | ||
Task evaluateViolations) { | ||
Violations violations = violationsContainer.maybeCreate('ktlint') | ||
return new KtlintConfigurator(project, violations, evaluateViolations) | ||
} | ||
|
||
KtlintConfigurator(Project project, Violations violations, Task evaluateViolations) { | ||
this.project = project | ||
this.violations = violations | ||
this.evaluateViolations = evaluateViolations | ||
this.variantFilter = new VariantFilter(project) | ||
} | ||
|
||
@Override | ||
void execute() { | ||
project.extensions.findByType(StaticAnalysisExtension).ext.ktlint = { Closure config -> | ||
if (!project.plugins.hasPlugin(KTLINT_PLUGIN)) { | ||
throw new GradleException(KTLINT_NOT_APPLIED) | ||
} | ||
|
||
def ktlint = project.ktlint | ||
ktlint.ignoreFailures = true | ||
ktlint.reporters = ['CHECKSTYLE', 'PLAIN'] | ||
ktlint.ext.includeVariants = { Closure<Boolean> filter -> | ||
variantFilter.includeVariantsFilter = filter | ||
} | ||
config.delegate = ktlint | ||
config() | ||
|
||
project.afterEvaluate { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just a notice: Had to add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should be ok, at least for now, since as you say that's what the plugin we're wrapping does |
||
|
||
project.plugins.withId("kotlin") { | ||
configureKotlinProject() | ||
} | ||
project.plugins.withId("kotlin2js") { | ||
configureKotlinProject() | ||
} | ||
project.plugins.withId("kotlin-platform-common") { | ||
configureKotlinProject() | ||
} | ||
project.plugins.withId('com.android.application') { | ||
configureAndroidWithVariants(variantFilter.filteredApplicationVariants) | ||
} | ||
project.plugins.withId('com.android.library') { | ||
configureAndroidWithVariants(variantFilter.filteredLibraryVariants) | ||
} | ||
} | ||
} | ||
} | ||
|
||
private void configureKotlinProject() { | ||
project.sourceSets.each { configureKtlint(it) } | ||
} | ||
|
||
private void configureAndroidWithVariants(def mainVariants) { | ||
mainVariants.all { configureKtlint(it) } | ||
variantFilter.filteredTestVariants.all { configureKtlint(it) } | ||
variantFilter.filteredUnitTestVariants.all { configureKtlint(it) } | ||
} | ||
|
||
private void configureKtlint(def sourceSet) { | ||
def collectViolations = createCollectViolationsTask(violations, sourceSet.name) | ||
evaluateViolations.dependsOn collectViolations | ||
} | ||
|
||
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") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No way to change the output in the plugin. Hardcoded for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean we get There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
task.htmlReportFile = new File(project.buildDir, "reports/ktlint/ktlint-${sourceSetName}.txt") | ||
task.violations = violations | ||
task.dependsOn project.tasks["ktlint${sourceSetName.capitalize()}Check"] | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
data class ValidClass(val prop1: String, val prop2: Int) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
class KtLintViolator { | ||
|
||
override fun equals(other: Any?): Boolean { | ||
// this is not allowed | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is not allowed? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha I think always returning true is not allowed. This is just a fixture and a copy paste from detekt fixtures. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, I need any Kotlin code with style problem. Would be happy to change. 👍 |
||
return true; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
package com.novoda.staticanalysis.internal.ktlint | ||
|
||
import com.novoda.test.Fixtures | ||
import com.novoda.test.TestProject | ||
import com.novoda.test.TestProjectRule | ||
import org.junit.Rule | ||
import org.junit.Test | ||
import org.junit.runner.RunWith | ||
import org.junit.runners.Parameterized | ||
|
||
import static com.novoda.test.LogsSubject.assertThat | ||
|
||
@RunWith(Parameterized.class) | ||
class KtlintIntegrationTest { | ||
|
||
private static final String KTLINT_NOT_APPLIED = 'The Ktlint plugin is configured but not applied. Please apply the plugin in your build script.' | ||
public static final String DEFAULT_CONFIG = ''' | ||
ktlint { | ||
includeVariants { it.name == "debug" } | ||
} | ||
''' | ||
public static final String EMPTY_CONFIG = 'ktlint {}' | ||
|
||
@Parameterized.Parameters(name = '{0}') | ||
static def rules() { | ||
return [ | ||
[TestProjectRule.forKotlinProject(), 'main'].toArray(), | ||
[TestProjectRule.forAndroidKotlinProject(), 'debug'].toArray() | ||
] | ||
} | ||
|
||
@Rule | ||
public final TestProjectRule projectRule | ||
private final String sourceSetName; | ||
|
||
KtlintIntegrationTest(TestProjectRule projectRule, String sourceSetName) { | ||
this.projectRule = projectRule | ||
this.sourceSetName = sourceSetName | ||
} | ||
|
||
@Test | ||
void shouldNotFailWhenKtlintIsNotConfigured() { | ||
def result = createProjectWith(Fixtures.Ktlint.SOURCES_WITH_ERROR) | ||
.build('evaluateViolations') | ||
|
||
assertThat(result.logs).doesNotContainKtlintViolations() | ||
} | ||
|
||
@Test | ||
void shouldFailBuildOnConfigurationWhenKtlintConfiguredButNotApplied() { | ||
def result = projectRule.newProject() | ||
.withToolsConfig(EMPTY_CONFIG) | ||
.buildAndFail('evaluateViolations') | ||
|
||
assertThat(result.logs).contains(KTLINT_NOT_APPLIED) | ||
} | ||
|
||
@Test | ||
void shouldFailBuildWhenKtlintErrorsOverTheThreshold() { | ||
def result = createProjectWith(Fixtures.Ktlint.SOURCES_WITH_ERROR) | ||
.withToolsConfig(DEFAULT_CONFIG) | ||
.buildAndFail('evaluateViolations') | ||
|
||
assertThat(result.logs).containsLimitExceeded(1, 0) | ||
assertThat(result.logs).containsKtlintViolations(1, | ||
result.buildFileUrl("reports/ktlint/ktlint-${sourceSetName}.txt")) | ||
} | ||
|
||
@Test | ||
void shouldNotFailWhenErrorsAreWithinThreshold() { | ||
def result = createProjectWith(Fixtures.Ktlint.SOURCES_WITH_ERROR, 1) | ||
.withToolsConfig(DEFAULT_CONFIG) | ||
.build('evaluateViolations') | ||
|
||
assertThat(result.logs).containsKtlintViolations(1, | ||
result.buildFileUrl("reports/ktlint/ktlint-${sourceSetName}.txt")) | ||
} | ||
|
||
@Test | ||
void shouldNotFailBuildWhenNoErrorsEncounteredAndNoThresholdTrespassed() { | ||
def result = createProjectWith(Fixtures.Ktlint.SOURCES_NO_ERROR, 0) | ||
.withToolsConfig(EMPTY_CONFIG) | ||
.build('evaluateViolations') | ||
|
||
assertThat(result.logs).doesNotContainLimitExceeded() | ||
assertThat(result.logs).doesNotContainKtlintViolations() | ||
} | ||
|
||
private TestProject createProjectWith(File sources, int maxErrors = 0) { | ||
projectRule.newProject() | ||
.withPlugin('org.jlleitschuh.gradle.ktlint', '4.1.0') | ||
.withSourceSet('main', sources) | ||
.withPenalty("""{ | ||
maxWarnings = 0 | ||
maxErrors = ${maxErrors} | ||
}""") | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,8 @@ buildscript { | |
} | ||
} | ||
plugins { | ||
${formatPlugins(project)} | ||
${formatPlugins(project)} | ||
id 'com.novoda.static-analysis' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is better if the |
||
} | ||
repositories { | ||
google() | ||
|
@@ -58,7 +59,7 @@ ${formatExtension(project)} | |
.collect { Map.Entry<String, List<String>> entry -> | ||
"""$entry.key { | ||
manifest.srcFile '${Fixtures.ANDROID_MANIFEST}' | ||
kotlin { | ||
java { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Android projects uses |
||
${entry.value.collect { "srcDir '$it'" }.join('\n\t\t\t\t')} | ||
} | ||
}""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry I actually read it, but somehow missed the connection 🙈 |
||
} | ||
repositories { | ||
jcenter() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,9 +15,15 @@ buildscript { | |
|
||
plugins { | ||
${formatPlugins(project)} | ||
id 'com.novoda.static-analysis' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same as above. |
||
} | ||
|
||
apply plugin: 'kotlin' | ||
|
||
repositories { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was needed for |
||
jcenter() | ||
} | ||
|
||
sourceSets { | ||
${formatSourceSets(project)} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this to make the task better cacheable. Don't think it will hurt in any way. WDYT?