Add task class for KtlintCheck #90

wolfs · 2018-04-26T16:02:51Z

This PR adds an actual task class for the ktlint check task. This allows declaring outputs for configured reports.

The task class does not yet support the old report command line arguments (single report only).
Moreover, it currently works only on Gradle 4.7, since it uses @Nested on a Map for declaring the outputs of the report.

If the plugin maintainers think this is a good direction, then I can address the remaining problems like supporting older ktlint versions or older Gradle versions.

This PR addresses part of #85. I ended up not using source task since it does not provide any benefits in this simple case.

This allows declaring the outputs for the configured reports. The task class does not yet support the old report command line arguments (single report only).

eskatos · 2018-04-26T16:07:38Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/KtlintCheck.kt

+import org.jlleitschuh.gradle.ktlint.reporter.KtlintReport
+import org.jlleitschuh.gradle.ktlint.reporter.ReporterType
+
+open class KtlintCheck : DefaultTask() {


⭕ Implementing VerificationTask would make sense

Implementing VerificationTask would somewhat force me to drop the Property<Boolean> type for ignoreFailures. Then I would need to use convention mapping, right?

What about SourceTask then you'd get the filtering API for free.

My main problem with SourceTask is that I would need a way to turn the FileTree into something ktlint understands (e.g. <baseDir>/**/*.kt). I wouldn't know how to do this via the public API. This is why I opted for not extending SourceTask.

I could add the individual files as command line arguments to ktlint. Would that work? Or will the command line become to long?

I am also not sure if filtering provides much extra value for the ktlint task. Are you saying that a use case is to exclude some files from linting via includes/excludes?

About SourceTask, the extra use case of including/excluding individual files could be tackled later, as a separate PR.

About VerificationTask, that's unfortunate but I see your point and agree @wolfs

eskatos · 2018-04-26T16:08:35Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/KtlintCheck.kt

+    val sources: FileTree = sourceDirectories.asFileTree.matching { it.include("**/*.kt") }
+
+    @get:Input
+    val verbose: Property<Boolean> = project.objects.property(Boolean::class.javaObjectType)


❌ Boolean::class.java should be enough

There are many other instances of this below, not commented on each one

If I use Boolean::class.java I get:

Cannot get the value of a property of type boolean as the provider associated with this property returned a value of type java.lang.Boolean.

when running the tests.

Ah right, fair enough

How about simplifying this to:

private fun Project.booleanObjectProperty() = objects.property(Boolean::class.javaObjectType)

Just so we don't have this same logic repeated 5 times.

…asses

JLLeitschuh · 2018-04-27T17:11:02Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/KtlintPlugin.kt

+            android.set(target.provider { extension.android && SemVer.parse(extension.version) >= SemVer(0, 12, 0) })
+            ignoreFailures.set(target.provider { extension.ignoreFailures })
+            outputToConsole.set(target.provider { extension.outputToConsole
+            })


Weird formatting here.

JLLeitschuh · 2018-04-27T17:11:54Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/KtlintPlugin.kt

-            this.applyReporters(target, extension, sourceSetName)
+            classpath.setFrom(ktLintConfig)
+            sourceDirectories.setFrom(kotlinSourceDirectories)
+            verbose.set(target.provider { extension.verbose })


Not a huge fan of how you have to call set on all of these properties. Seems so un-kotlin like.

Currently, that is the recommended usage. Do you have some better ideas?

JLLeitschuh · 2018-04-27T17:13:11Z

plugin/src/test/kotlin/org/jlleitschuh/gradle/ktlint/KtlintPluginVersionTest.kt

@@ -38,6 +40,8 @@ class KtlintPluginVersionTest : AbstractPluginTest() {

    @Test
    fun `with ktlint version less than 0_20`() {
+        withCleanSources()


Put this in a @BeforeEach method?

They are still here.

They should be gone now.

JLLeitschuh

Some things to fixup. On the right track though.

wolfs · 2018-05-01T21:25:03Z

@JLLeitschuh Thank you for the review! I addressed/answered your comments.

How are the requirements regarding support for older Gradle/ktlint versions? What do I still need to address?

JLLeitschuh · 2018-05-01T21:25:27Z

I'd feel more comfortable merging this PR after we've come to some sort of conclusion WRT the discussion here: gradle/kotlin-dsl-samples#597

I just want to make sure that we are really committing to using the best practices regarding API we are exposing.

JLLeitschuh · 2018-05-01T21:27:00Z

How are the requirements regarding support for older Gradle/ktlint versions?

As I'm not currently even using this plugin myself, I'm going to defer to @Tapchicoma about this.

This change doesn't seem to be an API breaking change so I'm fine with a minor version bump on this one.

The setup method already takes care of this.

Tapchicoma · 2018-05-03T19:50:16Z

How are the requirements regarding support for older Gradle/KtLint versions?

I am thinking that at some point plugin should drop support for older versions of KtLint and Gradle, to simplify code/reduce effort of supporting it. We can do it in this PR as well.

@wolfs what is desired minimal supported versions of Gradle and KtLint in this PR?

This change doesn't seem to be an API breaking change so I'm fine with a minor version bump on this one.

Indeed it is not changing the API, but on the other hand will break update for those who is still using old Gradle wrapper/set older KtLint version in extension (I wonder if there such installations 🤔), so, IMHO, better major version bump.

Tapchicoma · 2018-05-03T19:54:22Z

@wolfs I see that KtLint mininal version is 0.11.1 that introduced multiple reporters - it is ok. As for Gradle 4.7 - it is questionable - people not updating Gradle wrapper so fast, so better to support older versions. Ideally starting 4.0.

JLLeitschuh · 2018-05-03T19:59:22Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/KtlintCheck.kt

+        project.javaexec {
+            it.classpath = classpath
+            it.main = "com.github.shyiko.ktlint.Main"
+            it.args(sourceDirectories.map { "${it.path}/**/*.kt" })


Include *.kts incase someone is linting a buildSrc project with script based plugins.

Do *.kts files lint well?

@wolfs according to pinterest/ktlint#19 since 0.3.0 release

Making that change sounds orthogonal to this PR.
Even though it's a very interesting one :)

wolfs · 2018-05-05T20:57:01Z

@Tapchicoma The PR is now compatible with Gradle 4.3. Being compatible with Gradle 4.0 is somewhat hard, since the methods/types for Propertys changed between Gradle 4.2 and Gradle 4.3.
Would Gradle 4.3 be OK?

Tapchicoma · 2018-05-07T18:41:48Z

@wolfs yes, Gradle 4.3 should be fine 👍

JLLeitschuh · 2018-05-08T14:13:38Z

@wolfs Are getting close to having this PR ready to be merged?

wolfs · 2018-05-08T14:36:07Z

@JLLeitschuh From my side, this is good to merge. What about your comment #90 (comment) ? Are you fine with the use of the Provider API for now/

JLLeitschuh · 2018-05-08T14:38:35Z

I'm fine using the Provider API for now, yes. Still not a huge fan.

Can you resolve the comment and we can merge this in? Please make a note of your changes in the changelog.

wolfs · 2018-05-11T10:52:13Z

@JLLeitschuh Comment resolved (now **/kts files are checked, too) and updated changelog.

JLLeitschuh · 2018-05-11T13:59:18Z

CHANGELOG.md

 ### Changed
  - Update Kotlin to 1.2.41 version
  - Update Gradle wrapper to 4.7 version
 ### Fixed
+### Removed
+  - KtLint versions prior to 0.10.0 are not supported any more


Should be: "anymore"

JLLeitschuh · 2018-05-11T13:59:39Z

CHANGELOG.md


 ## [3.3.0] - 2018-4-24
 ### Added
 - Check for spaces in output path for KtLint versions earlier
 then 0.20.0 (#83)
- - Use relative for input file path sensivity (#67)
+ - Use relative for input file path sensitivity (#67)


JLLeitschuh · 2018-05-11T17:44:44Z

@wolfs I just want to make sure that the feature added by this PR didn't get dropped in all the changes you made:
#89

wolfs · 2018-05-11T21:50:00Z

@JLLeitschuh Good catch. I re-added the ruleSets feature.

JLLeitschuh

Looks good! @Tapchicoma Take a final pass and feel free to merge.

JLLeitschuh · 2018-05-11T23:27:10Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/KtlintCheck.kt

+        project.javaexec {
+            it.classpath = classpath
+            it.main = "com.github.shyiko.ktlint.Main"
+            it.args(sourceDirectories.flatMap { dir -> KOTLIN_EXTENSIONS.map { extension -> "${dir.path}/**/*.$extension" } })


I'm curious why you don't just use the sources property you already have in scope for this?

Sources are all the Kotlin files in scope. This is possibly a big argument list. If you think Ktlink can happily cope with having all the to be checked .kt files as separate arguments on the commandline than that would be fine. I am somewhat concerned about the length of the command line - especially on Windows.

Totally fair.

Tapchicoma

Overall LGTM, small improvements and ready to merge.

Tapchicoma · 2018-05-13T17:59:04Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/KtlintCheck.kt

+import javax.inject.Inject
+
+@CacheableTask
+open class KtlintCheck @Inject constructor(objectFactory: ObjectFactory) : DefaultTask() {


Wow, a lot of annotation "magic" to write a task 😱

We love annotations ;)
Is there a less verbose way to do constructor injection in Kotlin?

Nope, this is the way to do constructor injection.

I'm actually really pleased that in Kotlin, constructor injection is actually simpler than field injection.

Because of this, it encourages developers to use Constructor injection.

Tapchicoma · 2018-05-13T18:08:48Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/KtlintCheck.kt

+                throw GradleException("Ktlint versions less than 0.10.0 are not supported. Detected Ktlint version: $it.")
+            }
+        }
+        ktlintVersion?.let { version ->


Better to move into private method for readability

Tapchicoma · 2018-05-13T18:09:17Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/KtlintCheck.kt

+    fun lint() {
+        val reportsToProcess = enabledReports.values
+        val ktlintVersion = determineKtlintVersion(classpath)
+        ktlintVersion?.let {


Better to move into private method for readability

Tapchicoma · 2018-05-13T18:47:22Z

plugin/src/main/kotlin/org/jlleitschuh/gradle/ktlint/KtlintCheck.kt

+        val ktlintJarRegex = "ktlint-(\\d+.*)\\.jar".toRegex()
+        val ktlintJarName = ktlintClasspath.find { ktlintJarRegex.matches(it.name) }?.name
+        val version = ktlintJarName?.let {
+            val versionString = ktlintJarRegex.matchEntire(it)!!.groups[1]!!.value


Better to replace with:

val versionString = ktlintJarRegex.matchEntire(it)?.groups[1]?.value ?: return null

Tapchicoma · 2018-05-13T19:12:18Z

CHANGELOG.md

 ### Changed
  - Update Kotlin to 1.2.41 version
  - Update Gradle wrapper to 4.7 version
 ### Fixed
+### Removed
+  - KtLint versions prior to 0.10.0 are not supported anymore


Please also add that Gradle is supported from version 4.3

Tapchicoma · 2018-05-14T19:16:43Z

@wolfs thanks for implementing this changes!

wolfs added 3 commits April 26, 2018 17:57

Upgradle to 4.7

8a24241

Add task class for KtlintCheck

7a7b1e3

This allows declaring the outputs for the configured reports. The task class does not yet support the old report command line arguments (single report only).

Skip ktlint check if it has no sources

7d0f208

eskatos reviewed Apr 26, 2018

View reviewed changes

wolfs added 3 commits April 27, 2018 09:51

Remove unnecessary --info

9309963

Merge remote-tracking branch 'upstream/master' into wolfs/use-task-cl…

6d0768c

…asses

Add check for spaces in output files

ef60714

wolfs force-pushed the wolfs/use-task-classes branch from 120d29c to ef60714 Compare April 27, 2018 13:20

JLLeitschuh reviewed Apr 27, 2018

View reviewed changes

JLLeitschuh requested changes Apr 27, 2018

View reviewed changes

wolfs added 2 commits May 1, 2018 22:58

Merge branch 'master'

57149b9

Address review comments

183fb24

Remove unnecessary withCleanSources call

8618b75

The setup method already takes care of this.

JLLeitschuh reviewed May 3, 2018

View reviewed changes

Make task compatible with Gradle 4.3

ad1b62e

wolfs added 3 commits May 6, 2018 22:59

Make KtlintCheck cacheable

fe926e0

Only support Ktlint versions starting from 0.10.0

ba10a32

Use extensions for test methods

eba6955

Also check *.kts files

c9f3aeb

uzzu mentioned this pull request May 10, 2018

Introduce ruleset option #89

Merged

wolfs added 2 commits May 11, 2018 12:49

Update changelog

97d5552

Merge branch 'master'

9702936

Extract kt/kts to constant

0cfbee0

JLLeitschuh reviewed May 11, 2018

View reviewed changes

Fix changelog: any more -> anymore

56509e2

Re-introduce lost ruleSets property

7d5e27b

wolfs force-pushed the wolfs/use-task-classes branch from 4de3efc to 7d5e27b Compare May 11, 2018 21:49

JLLeitschuh approved these changes May 11, 2018

View reviewed changes

Tapchicoma requested changes May 13, 2018

View reviewed changes

Address review feedback

6766eaa

Tapchicoma approved these changes May 14, 2018

View reviewed changes

Tapchicoma merged commit 788963c into JLLeitschuh:master May 14, 2018

wolfs mentioned this pull request May 15, 2018

ktlint check tasks should be cacheable #67

Closed

Tapchicoma mentioned this pull request May 28, 2018

Make the Ktlint task a SourceTask #85

Closed

Add task class for KtlintCheck #90

Add task class for KtlintCheck #90

Conversation

wolfs commented Apr 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh Apr 27, 2018 • edited Loading

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

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

JLLeitschuh left a comment

Choose a reason for hiding this comment

wolfs commented May 1, 2018

JLLeitschuh commented May 1, 2018

JLLeitschuh commented May 1, 2018

Tapchicoma commented May 3, 2018

Tapchicoma commented May 3, 2018 • edited Loading

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

wolfs commented May 5, 2018

Tapchicoma commented May 7, 2018

JLLeitschuh commented May 8, 2018

wolfs commented May 8, 2018

JLLeitschuh commented May 8, 2018

wolfs commented May 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JLLeitschuh commented May 11, 2018

wolfs commented May 11, 2018 • edited Loading

JLLeitschuh 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

Tapchicoma 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

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

Tapchicoma commented May 14, 2018

wolfs commented Apr 26, 2018 •

edited

Loading

JLLeitschuh Apr 27, 2018 •

edited

Loading

Tapchicoma commented May 3, 2018 •

edited

Loading

wolfs commented May 11, 2018 •

edited

Loading