Adds ktlint linting for Gradle Kotlin DSL files

[JLLeitschuh]

Closes #115

[JLLeitschuh]

I was unable to try creating a build.gradle.kts file to lint as part of a test because your test system seems to be using the 2.14 gradle test library.

[nedtwigg]

Great start! I've got a couple concerns:

findCompanionScripts and gradle folder

It's relatively common for folks to have a gradle folder in their root directory, and for that folder to contain shared scripts. Looks like this PR doesn't handle that case well.

setIncludeSubprojects and files outside of the current project

This PR takes an approach moves work from child projects into the root project. Here's a usecase where that goes bad.

build.gradle
projA/build.gradle
projB/build.gradle

I'm doing work in projA. I know that I've mucked up formatting, so I run gradle :projA:spotlessApply. Hmm, why is projA/build.gradle still mucked up? The code got formatted, but not the build?

convention-over-configuration vs docs

I'm all for the convention-over-configuration and sensible defaults, but I think this takes things a bit too far. I think it's easier for users to manually specify target '**/*.gradle.kts' than it is to figure out the various modes that this PR supports.

I think these magic modes should be removed, and the default behavior should just be *.gradle.kts.

[JLLeitschuh]

Fair enough.

findCompanionScripts and gradle folder

It's relatively common for folks to have a gradle folder in their root directory, and for that folder to contain shared scripts. Looks like this PR doesn't handle that case well.

That use case would be handled by default.

I understand your concerns. I can clean it up and remove the "magic" modes. Should the expectation be that the user will just apply the configuration to the root project and be done with it?

Also, are you fine with the name? kotlinBuild? Or perhaps it should be gradleKotlin?

[nedtwigg]

Should the expectation be that the user will just apply the configuration to the root project and be done with it?

The expectation should be that the plugin will act only on the project it is applied to. Here's an example usage:

allprojects {
    spotless {
        kotlinGradle {
            ktlint()
        }
        kotlin {
             ktlint()
             licenseHeaderFile 'someHeaderFile'
        }
    }
}

Or maybe

allprojects {
    spotless {
        kotlinGradle {
            ktlint()
        }
    }
}
subprojects {
    spotless {
        kotlin {
             ktlint()
             licenseHeaderFile 'someHeaderFile'
        }
    }
}

Re: name, I see this as a specialization of the kotlin extension, so I think it makes sense for the name to reflect that, kotlinGradle. If we were to do something similar for groovy, groovyGradle would be a natural fit. For now, I don't see demand for it on the groovy side.

[JLLeitschuh]

The expectation should be that the plugin will act only on the project it is applied to.

So how would this work if I have extra scripts in some gradle or extraScripts directory? Which project owns that extra script? The project that they are nested within?

How would you tell the difference between a build.gradle.kts file in a subproject vs an extra script in a subdirectory but that subdirectory isn't a self contained project?

[JLLeitschuh]

TODO list:

• Change kotlinBuild to kotlinGradle

[nedtwigg]

So how would this work if I have extra scripts in some gradle or extraScripts directory?

Here's one way, where only the root project formats the shared scripts

spotless {
    kotlinGradle {
        target '*.gradle.kts', 'extraScripts/*.gradle.kts'
    }
}
subprojects {
    spotless {
        kotlinGradle {
            // default target of *.gradle.kts works
        }
    }
}

Here's another way, where every project formats the shared files

allprojects {
    spotless {
        kotlinGradle {
            target files('*.gradle.kts'), rootProject.files('extraScripts/*.gradle.kts')
            ktlint()
        }
    }
}

I'm all about convention-over-configuration, but the "gradle shared scripts" folder isn't fully standardized, so I don't see a good way to handle this without manually specifying the target. Both approaches above are fairly easy to read and self-documenting. I see that as an advantage over a special-purpose addSharedScripts('') method in the DSL that users will have to discover and understand.

[JLLeitschuh]

So just to make sure I'm 100% sure what you are saying: *.gradle.kts would be the default for the plugin configuration?
If users want to override the target default then they can do so?

[nedtwigg]

Yep. That's the way all our language-specific formats work. In the setupTask method, we check to see if the user has already set a target, in which case we just go with what they set. If they didn't set anything, then we assign a useful default.

[JLLeitschuh]

There's no good way to determine the top of a .gradle.kts file so I removed the licenseHeader helpers as I realized that the delimiter wasn't right.

[JLLeitschuh]

I had files showing up that should be ignored because of intellij. I can move this to a different PR if you really want but I figured it wasn't worth the overhead. (It's in its own commit).

[nedtwigg]

Great! Only changes left are:

• rename KotlinBuildExtension/Test to KotlinGradleExtension/Test (to match the extension)
• update plugin-gradle/CHANGES.md, plugin-gradle/README.md
• credit yourself as a new contributor in README.md.

[JLLeitschuh]

I can't check those boxes 😢 but I did make the changes you requested.

[nedtwigg]

@JLLeitschuh does this look okay to you?

[JLLeitschuh]

Looks good. It does still support licenseHeader header, delimiter because the task extends from FormatExtension but I leave it as an exercise to the user to create a wacky regex to find the top of the file.

[nedtwigg]

Agreed :)

[JLLeitschuh]

Awesome! Thanks!

[fvgh]

@nedtwigg Sorry to raise the issue again, but isn't the licenseHeader statement contradicting what we discussed before.
Shouldn't it be OK for most users to configure the import as a delimiter?

[nedtwigg]

Always happy to discuss anything further :)

Shouldn't it be OK for most users to configure the import as a delimiter?

It is OK, and still OK. The primary audience for the README is

1. new user trying to setup Spotless
2. an existing user trying to add more functionality

It doesn't need to be as complete as the full javadoc. I think sending new users down the regex rabbithole discourages them from the easy wins that Spotless has to offer, by forcing them to face down the full complexity of what is possible right from the start.

If I wanted to format the gradle scripts for my main project, I would need to handle files that start with plugins {, buildscript {, import something, // some comment, /* some comment, and even def someVar = someValue. The general case is a mess. We give users the power to tackle that mess if they want, but I don't think we need to encourage it in the README.

[fvgh]

Ok, fair enough. Thx for the explanation. Found it funny that you explicitly ruled it out in the ReadMe. Will provide something similar for Groovy.

[nedtwigg]

I agree it's weird, from an expert perspective, to explicitly rule it out. The kotlin example right above it, we have two easy things to offer users - ktlint and license headers. So in the kotlinGradle example, a first time user would reasonably think "great, I'll use ktlint and license there too!" But using the license is much harder, so the right thing for a beginner is to take the easy wins and move on.