Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add task to generate pre commit hook that formats staged changes #270

Open
wants to merge 23 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 22 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 0 additions & 7 deletions .idea/misc.xml

This file was deleted.

20 changes: 13 additions & 7 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,11 @@ Please note that `ktfmt-gradle` relies on `ktfmt` hence the minimum supported JD

Please also note the following requirements:

* **Kotlin 1.4+**. In order to reformat Kotlin 1.4 code, you need run on **Gradle to 6.8+** (This is due to Gradle 6.7 embedding Kotlin 1.3.x - See [#12660](https://github.com/gradle/gradle/issues/12660)).
* **Kotlin 1.4+**. In order to reformat Kotlin 1.4 code, you need run on **Gradle to 6.8+** (This is due to Gradle 6.7
embedding Kotlin 1.3.x - See [#12660](https://github.com/gradle/gradle/issues/12660)).

* **Android**. `ktfmt-gradle` relies on features from **Android Gradle Plugin 4.1+**. So make sure you bump AGP before applying this plugin.
* **Android**. `ktfmt-gradle` relies on features from **Android Gradle Plugin 4.1+**. So make sure you bump AGP before
applying this plugin.
Comment on lines +44 to +48
Copy link
Owner

Choose a reason for hiding this comment

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

Can you amend this and other unrelated changes to the README?

Copy link
Author

Choose a reason for hiding this comment

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

Sure, its because running the formatter.


### Task

Expand Down Expand Up @@ -91,10 +93,10 @@ To enable different styles you can simply:
ktfmt {
// Dropbox style - 4 space indentation
dropboxStyle()

// Google style - 2 space indentation & automatically adds/removes trailing commas
googleStyle()

// KotlinLang style - 4 space indentation - From kotlinlang.org/docs/coding-conventions.html
kotlinLangStyle()
}
Expand All @@ -121,7 +123,8 @@ ktfmt {

You can leverage the `--include-only` to let ktfmt-gradle run only on a specific subset of files.

To this you can register a simple task of type `KtfmtCheckTask` or `KtfmtFormatTask` in your `build.gradle.kts` as follows:
To this you can register a simple task of type `KtfmtCheckTask` or `KtfmtFormatTask` in your `build.gradle.kts` as
follows:

```kotlin
import com.ncorti.ktfmt.gradle.tasks.*
Expand All @@ -132,10 +135,13 @@ tasks.register<KtfmtFormatTask>("ktfmtPrecommit") {
}
```

You can then invoke the task with `--include-only` and a comma separated list of relative path of files:
Copy link
Owner

Choose a reason for hiding this comment

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

If we remove this sentence, then we would have to remove the entire section above

Copy link
Author

Choose a reason for hiding this comment

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

You are probably right, my only thought is that you can still use a custom task without generating the hook. So i believe i should bring it back instead

## Generating with a pre-commit hook 🎣

You can also generate a generate a git pre-commit hook which will run `ktfmtFormat` on your commits using the following
command:

```
./gradlew ktfmtPrecommit --include-only=src/main/java/File1.kt:src/main/java/File2.kt
./gradlew ktfmtGenerateGitPreCommitHook
```

The task will execute only on the file you passed and will skip all the others.
Expand Down
6 changes: 6 additions & 0 deletions plugin-build/plugin/api/plugin.api
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ public abstract class com/ncorti/ktfmt/gradle/KtfmtPlugin : org/gradle/api/Plugi
public final class com/ncorti/ktfmt/gradle/KtfmtPlugin$Companion {
}

public abstract class com/ncorti/ktfmt/gradle/tasks/CreateGitPreCommitHookTask : org/gradle/api/DefaultTask {
public fun <init> ()V
public final fun createHook ()V
public abstract fun getWorkerExecutor ()Lorg/gradle/workers/WorkerExecutor;
}

public abstract class com/ncorti/ktfmt/gradle/tasks/KtfmtBaseTask : org/gradle/api/tasks/SourceTask {
protected abstract fun execute (Lorg/gradle/workers/WorkQueue;)V
public abstract fun getIncludeOnly ()Lorg/gradle/api/provider/Property;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import com.ncorti.ktfmt.gradle.KtfmtPluginUtils.EXTENSION_NAME
import com.ncorti.ktfmt.gradle.KtfmtPluginUtils.TASK_NAME_CHECK
import com.ncorti.ktfmt.gradle.KtfmtPluginUtils.TASK_NAME_FORMAT
import com.ncorti.ktfmt.gradle.KtfmtPluginUtils.createTasksForSourceSet
import com.ncorti.ktfmt.gradle.KtfmtPluginUtils.registerGitPreCommitHookTask
import com.ncorti.ktfmt.gradle.tasks.KtfmtBaseTask
import com.ncorti.ktfmt.gradle.util.i
import org.gradle.api.Plugin
Expand Down Expand Up @@ -78,6 +79,9 @@ abstract class KtfmtPlugin : Plugin<Project> {

private fun applyKtfmt(project: Project) {
val extension = project.extensions.getByType(KotlinProjectExtension::class.java)

registerGitPreCommitHookTask(project)

extension.sourceSets.all {
createTasksForSourceSet(
project,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.ncorti.ktfmt.gradle

import com.ncorti.ktfmt.gradle.tasks.CreateGitPreCommitHookTask
import com.ncorti.ktfmt.gradle.tasks.KtfmtCheckTask
import com.ncorti.ktfmt.gradle.tasks.KtfmtFormatTask
import org.gradle.api.Project
Expand Down Expand Up @@ -109,13 +110,31 @@ internal object KtfmtPluginUtils {
}
val taskName = "$TASK_NAME_FORMAT$srcSetName"
val inputDirs = srcDir.toList()

// check if specific files are specified to be included. If not, use the default includes
val includes =
(project.findProperty("ktfmt.gradle.includeOnly") as String?)
Copy link
Owner

Choose a reason for hiding this comment

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

You're not handling the interaction between this Gradle property AND the use also specifying a --include-only.

I think instead of passing the setIncludes here, we should read the Gradle Preperty inside KtfmtBaseTask. So the logic would be:

  1. If you specified a --include-only flag, we honor it.
  2. If not, if you specified the "ktfmt.gradle.includeOnly" property, we honor it
  3. Otherwise we just process the default includes

Copy link
Author

Choose a reason for hiding this comment

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

I see, makes sense. I will move it to the task while keeping the same significance order

?.split(",")
?.filter { file -> file.isNotEmpty() }
?.takeIf { files -> files.isNotEmpty() } ?: KtfmtPlugin.defaultIncludes
Copy link
Owner

Choose a reason for hiding this comment

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

I thought I left a comment here but I apparently I haven't (couldn't find it anymore).

My question is: have you tried this? As setIncludes below expects a glob, and not a list of files. If you see:

val defaultIncludes = listOf("**/*.kt", "**/*.kts")

so I'm unsure how it ends up working if you pass a list of files.


return project.tasks.register(taskName, KtfmtFormatTask::class.java) {
it.description =
"Run Ktfmt formatter validation for sourceSet '$name' on project '${project.name}'"

it.setSource(inputDirs)
it.setIncludes(KtfmtPlugin.defaultIncludes)
it.setIncludes(includes)
it.setExcludes(KtfmtPlugin.defaultExcludes)
it.bean = ktfmtExtension.toBean()
}
}

internal fun registerGitPreCommitHookTask(project: Project) {
project.tasks.register(
"ktfmtGenerateGitPreCommitHook",
CreateGitPreCommitHookTask::class.java
) {
it.description = "Creates a git hook that runs ktfmt before commit"
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
package com.ncorti.ktfmt.gradle.tasks

import com.ncorti.ktfmt.gradle.tasks.worker.PreCommitHookFormatAction
import javax.inject.Inject
import org.gradle.api.DefaultTask
import org.gradle.api.tasks.TaskAction
import org.gradle.workers.WorkerExecutor

/** ktfmt-gradle Git hook task. Creates a git hook that runs ktfmt on git commit */
abstract class CreateGitPreCommitHookTask : DefaultTask() {
@Inject abstract fun getWorkerExecutor(): WorkerExecutor

@TaskAction
fun createHook() {
val workQueue = getWorkerExecutor().noIsolation()
workQueue.submit(PreCommitHookFormatAction::class.java) { parameters ->
parameters.projectDir.set(project.projectDir)
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
package com.ncorti.ktfmt.gradle.tasks.worker

import com.ncorti.ktfmt.gradle.util.e
import com.ncorti.ktfmt.gradle.util.i
import com.ncorti.ktfmt.gradle.util.w
import java.io.File
import org.gradle.api.file.DirectoryProperty
import org.gradle.api.logging.Logger
import org.gradle.api.logging.Logging
import org.gradle.workers.WorkAction
import org.gradle.workers.WorkParameters
import org.jetbrains.kotlin.incremental.createDirectory

internal abstract class PreCommitHookFormatAction :
WorkAction<PreCommitHookFormatAction.PreCommitHookParameters> {

interface PreCommitHookParameters : WorkParameters {
val projectDir: DirectoryProperty
}

protected val logger: Logger = Logging.getLogger(PreCommitHookFormatAction::class.java)

override fun execute() {
val rootDirectory = parameters.projectDir.get().asFile
val gitDirectory = File(rootDirectory, ".git")
val hooksDirectory = File(gitDirectory, "hooks")

if (!gitDirectory.exists()) {
logger.e("Git directory not found, will not create the hook")
return
}

if (!hooksDirectory.exists()) {
logger.i("Hooks directory does not exist, will create one")
hooksDirectory.createDirectory()
}

val hookFile = File(hooksDirectory, "pre-commit")

if (hookFile.exists()) {
logger.w("Pre commit hook file already exists, aborting hook generation")
} else {
hookFile.createNewFile()

hookFile.writeText(
"""
#!/bin/bash
staged_files_count=${'$'}(git diff --name-only --cached --numstat -- '*.kt' | wc -l)

# format only if there are kotlin files in git's index
if [[ ${'$'}staged_files_count -gt 0 ]]; then
${rootDirectory.path}/gradlew -Pktfmt.gradle.includeOnly=${'$'}(git diff --name-only --cached -- '*.kt' | paste -sd ",") ktfmtFormat;
git add -A $(git diff --name-only --cached -- '*.kt')
fi
exit 0;
"""
.trimIndent()
)

logger.i("Ktfmt pre commit hook was created")
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
package com.ncorti.ktfmt.gradle.tasks

import com.google.common.truth.Truth.assertThat
import java.io.File
import org.gradle.testkit.runner.GradleRunner
import org.gradle.testkit.runner.TaskOutcome
import org.jetbrains.kotlin.gradle.internal.ensureParentDirsCreated
import org.jetbrains.kotlin.incremental.createDirectory
import org.junit.jupiter.api.BeforeEach
import org.junit.jupiter.api.Test
import org.junit.jupiter.api.io.TempDir

class GitPreCommitHookIntegrationTest {
@TempDir lateinit var tempDir: File

@BeforeEach
fun setUp() {
File(tempDir, "src/main/java").mkdirs()
File("src/test/resources/jvmProject").copyRecursively(tempDir)
}

@Test
fun `it does not create the pre commit hook if git directory does not exist`() {
val result =
GradleRunner.create()
.withProjectDir(tempDir)
.withPluginClasspath()
.withArguments("ktfmtGenerateGitPreCommitHook")
.build()

assertThat(result.task(":ktfmtGenerateGitPreCommitHook")?.outcome)
.isEqualTo(TaskOutcome.SUCCESS)
assertThat(result.output)
.contains("[ktfmt] Git directory not found, will not create the hook")
assertThat(File("${tempDir.path}/.git/hooks/pre-commit").exists()).isFalse()
}

@Test
fun `it does create the pre commit hook if git directory exists`() {
val gitDirectory = File("${tempDir.path}/.git")
gitDirectory.createDirectory()

val result =
GradleRunner.create()
.withProjectDir(tempDir)
.withPluginClasspath()
.withArguments("ktfmtGenerateGitPreCommitHook")
.build()

assertThat(result.task(":ktfmtGenerateGitPreCommitHook")?.outcome)
.isEqualTo(TaskOutcome.SUCCESS)
assertThat(File("${tempDir.path}/.git/hooks/pre-commit").exists()).isTrue()

val hookContent = File("${tempDir.path}/.git/hooks/pre-commit").readText().split("\n")

assertThat(hookContent[1])
.isEqualTo(
"staged_files_count=$(git diff --name-only --cached --numstat -- '*.kt' | wc -l)"
)
assertThat(hookContent[4]).isEqualTo("if [[ ${'$'}staged_files_count -gt 0 ]]; then")
assertThat(hookContent[5])
.contains(
"""
gradlew -Pktfmt.gradle.includeOnly=${'$'}(git diff --name-only --cached -- '*.kt' | paste -sd ",") ktfmtFormat;
"""
.trimIndent()
)
assertThat(hookContent[6]).contains("git add -A $(git diff --name-only --cached -- '*.kt')")
}

@Test
fun `it does not create the pre commit hook if git directory exists but hook also already exists`() {
val gitDirectory = File("${tempDir.path}/.git")

gitDirectory.createDirectory()

val hookFile = File("${tempDir.path}/.git/hooks/pre-commit")

hookFile.ensureParentDirsCreated()
hookFile.createNewFile()

hookFile.writeText(
"""
#!/bin/bash
echo exists;
"""
.trimIndent()
)

val result =
GradleRunner.create()
.withProjectDir(tempDir)
.withPluginClasspath()
.withArguments("ktfmtGenerateGitPreCommitHook")
.build()

assertThat(result.task(":ktfmtGenerateGitPreCommitHook")?.outcome)
.isEqualTo(TaskOutcome.SUCCESS)
assertThat(File("${tempDir.path}/.git/hooks/pre-commit").exists()).isTrue()
assertThat(result.output)
.contains("[ktfmt] Pre commit hook file already exists, aborting hook generation")

assertThat(hookFile.readText())
.isEqualTo(
"""
#!/bin/bash
echo exists;
"""
.trimIndent()
)

// clean up
gitDirectory.delete()
}
}
Loading