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

gpopides · 2024-03-19T20:21:14Z

🚀 Description

I have added 2 tasks. 1 is intented to be used by the user (ktfmtGeneratePreCommitHook) and 1 internal (ktfmtPreCommitHook).

ktfmtPreCommitHook Acts pretty much the same with the example in the readme. The pre commit hook will use this task to format staged kotlin files. I have added this task in order to avoid asking the user to add the pre commit example that is present in the readme.

To generate the file, the user needs to run gradlew ktfmtGeneratePreCommitHook

The hook will be generated if :

There is a git directory in the root directory of the project
There isnt an existing pre-commit hook in the hooks directory

📄 Motivation and Context

This PR tries to address this issue about generating a git pre commit hook for running ktformat on commit

🧪 How Has This Been Tested?

Integration tests + manual tests on a different project.

Tested on Arch linux with Kotlin 2.0.0-Beta4 and Gradle 8.6

📦 Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)

✅ Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.

cortinico · 2024-03-19T22:27:49Z

Thanks for sending this over @gpopides
The unit tests are failing, can we make them green?

gpopides · 2024-03-19T22:29:05Z

Thanks for sending this over @gpopides The unit tests are failing, can we make them green?

Sure i will take a look again tomorrow. Locally they are passing but i will make sure it works on CI too!

cortinico · 2024-03-21T00:01:38Z

CI is still red apparentyl

gpopides · 2024-03-21T07:19:15Z

Indeed, the last failure was because of a detekt issue which i pushed a change to fix. This makes the macos and ubuntu builds pass but the windows one still fails. Looking at the artifact i see:

Execution failed for task ':ktfmtGenerateGitPreCommitHook'.
  > Cannot access input property 'source' of task ':ktfmtGenerateGitPreCommitHook'. Accessing unreadable inputs or outputs is not supported. Declare the task as untracked by using Task.doNotTrackState(). For more information, please refer to https://docs.gradle.org/8.6/userguide/incremental_build.html#sec:disable-state-tracking in the Gradle documentation.
   > Failed to create MD5 hash for file content.

Unfortunately i don't have a windows machine nor i am that familiar with it. Do you happen to have any insights on why this might happen?

Thanks

...build/plugin/src/main/java/com/ncorti/ktfmt/gradle/tasks/worker/PreCommitHookFormatAction.kt

cortinico · 2024-03-23T15:21:08Z

...build/plugin/src/main/java/com/ncorti/ktfmt/gradle/tasks/worker/PreCommitHookFormatAction.kt

+            hooksDirectory.createDirectory()
+        }
+
+        val hookFile = File("${hooksDirectory}/pre-commit")


same as above

cortinico · 2024-03-23T15:23:02Z

plugin-build/plugin/src/main/java/com/ncorti/ktfmt/gradle/KtfmtPluginUtils.kt

+        project.tasks.register("ktfmtPreCommitHook", KtfmtFormatTask::class.java) {
+            it.source = project.fileTree(project.projectDir)
+            it.doNotTrackState("Task does not require gradle cache")
+            it.include("**/*.kt")
+        }


This feels wrong as you're basically recreating a format task that will reformat everything no?

The idea behind this is that is based on the example in the readme which mentions that we can create a task to be used as pre commit. Here i tried to handle the case where the user has not added the specific task. I can remove it and check if the task (mentioned in the readme) is defined and use that. Do you think that's a better approach?

Ah I see. So first you can just use ktfmtFormat. We don't need an extra task.
The problem is that it won't work well for all the source set tasks.

Thanks for clarification. Just a note, trying to use ktfmtFormat does not accept specific files --include-only. What am i missing?

The problem is that it won't work well for all the source set tasks.

Yeah that's what I meant here.
Say you're applying ktfmt to a KMP project, you will have those tasks:

ktfmtFormatKmpAndroidTest - Run Ktfmt formatter validation for sourceSet 'kmp androidTest' on project 'example-kmp' ktfmtFormatKmpAndroidTestDebug - Run Ktfmt formatter validation for sourceSet 'kmp androidTestDebug' on project 'example-kmp' ktfmtFormatKmpAndroidTestRelease - Run Ktfmt formatter validation for sourceSet 'kmp androidTestRelease' on project 'example-kmp' ktfmtFormatKmpCommonMain - Run Ktfmt formatter validation for sourceSet 'kmp commonMain' on project 'example-kmp' ktfmtFormatKmpCommonTest - Run Ktfmt formatter validation for sourceSet 'kmp commonTest' on project 'example-kmp' ktfmtFormatKmpDebug - Run Ktfmt formatter validation for sourceSet 'kmp debug' on project 'example-kmp' ktfmtFormatKmpJvmMain - Run Ktfmt formatter validation for sourceSet 'kmp jvmMain' on project 'example-kmp' ktfmtFormatKmpJvmTest - Run Ktfmt formatter validation for sourceSet 'kmp jvmTest' on project 'example-kmp' ktfmtFormatKmpMain - Run Ktfmt formatter validation for sourceSet 'kmp main' on project 'example-kmp' ktfmtFormatKmpRelease - Run Ktfmt formatter validation for sourceSet 'kmp release' on project 'example-kmp' ktfmtFormatKmpTest - Run Ktfmt formatter validation for sourceSet 'kmp test' on project 'example-kmp' ktfmtFormatKmpTestDebug - Run Ktfmt formatter validation for sourceSet 'kmp testDebug' on project 'example-kmp' ktfmtFormatKmpTestFixtures - Run Ktfmt formatter validation for sourceSet 'kmp testFixtures' on project 'example-kmp' ktfmtFormatKmpTestFixturesDebug - Run Ktfmt formatter validation for sourceSet 'kmp testFixturesDebug' on project 'example-kmp' ktfmtFormatKmpTestFixturesRelease - Run Ktfmt formatter validation for sourceSet 'kmp testFixturesRelease' on project 'example-kmp' ktfmtFormatKmpTestRelease - Run Ktfmt formatter validation for sourceSet 'kmp testRelease' on project 'example-kmp'

The pre-commit hook will have to invoke those tasks and pass the --include-only flags to those

The alternative would be to use a Gradle Property, that you can set inside the commit source code as env variable to pass the --include-only values

Do you mean setting for example ktfmtFormatKmpMain,ktfmtFormatKmpTestFixtures as the env variable and passing the include files to these ?

Nope. I mean that we can instruct each task to read a Gradle property called ktfmt.gradle.includeOnly. And then you'll be able to set an env variable called ORG_GRADLE_PROJECT_ktfmt.gradle.includeOnly=... to specify the files to include.

More on this here:
https://docs.gradle.org/current/userguide/project_properties.html#sec:project_properties

Okay now its more clear to me. Just to verify, that will require changing createFormatTask to check this variable first and use it and if not present, use the existing defaultIncludes.

Which means that if we set this variable as you have said, we can use just ktfmtFormat

Is that correct?

Yeah exactly

cortinico · 2024-03-23T15:27:04Z

plugin-build/plugin/src/main/java/com/ncorti/ktfmt/gradle/tasks/CreateGitPreCommitHookTask.kt

+import org.gradle.workers.WorkerExecutor
+
+/** ktfmt-gradle Git hook task. Creates a git hook that runs ktfmt on git commit */
+abstract class CreateGitPreCommitHookTask : SourceTask() {


Suggested change

abstract class CreateGitPreCommitHookTask : SourceTask() {

abstract class CreateGitPreCommitHookTask : DefaultTask() {

@gpopides This is the cause of the Windows failure. This is not a Source Task as you're not populating the source property. Windows is failing while attepting to MD5 the source field which is null here

Thanks a lot for the feedback, i will change that.

…s/worker/PreCommitHookFormatAction.kt Co-authored-by: Nicola Corti <[email protected]>

cortinico

Thanks for the amazing work so far @gpopides

gpopides · 2024-03-24T19:55:28Z

I have added the include option as you suggested and have rebased your newest changes. Is there anything else that i have missed before going forward with this PR?

cortinico · 2024-03-26T00:51:27Z

README.md

+* **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.


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

Sure, its because running the formatter.

cortinico · 2024-03-26T00:52:01Z

README.md

@@ -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:


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

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

cortinico · 2024-03-26T00:56:36Z

plugin-build/plugin/src/main/java/com/ncorti/ktfmt/gradle/KtfmtPluginUtils.kt

+        // check if specific files are specified to be included. If not, use the default includes
+        val includes =
+            (project.findProperty("ktfmt.gradle.includeOnly") as String?)
+                ?.split(",")
+                ?.filter { file -> file.isNotEmpty() }
+                ?.takeIf { files -> files.isNotEmpty() } ?: KtfmtPlugin.defaultIncludes


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.

cortinico · 2024-03-26T00:59:56Z

plugin-build/plugin/src/main/java/com/ncorti/ktfmt/gradle/KtfmtPluginUtils.kt

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


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:

If you specified a --include-only flag, we honor it.

If not, if you specified the "ktfmt.gradle.includeOnly" property, we honor it

Otherwise we just process the default includes

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

cortinico requested changes Mar 23, 2024

View reviewed changes

cortinico reviewed Mar 23, 2024

View reviewed changes

gpopides and others added 21 commits March 23, 2024 21:11

Add task

3d61ce4

Add tests

32b1894

Rebase upstream

eb112e2

format

a5b945c

dump api file

e462857

Rename file

9464674

Remove test file path assertion from test

f8a6c50

Bring back assertion for hook content and add gradle annotations

b68f1e3

Make hook content assertion a partial assertion

03f32a8

Remove prints

6453a8b

Fix detekt issue

0132e9e

Format

db4cafd

Remove input directory from task

ea2deec

forgot to remote the variable set

ece3e8d

Dump again the api

9370dd2

Attempt to change the source of task

a48c055

Try to fix windows issue with doNotTrackState

cfbed62

Update plugin-build/plugin/src/main/java/com/ncorti/ktfmt/gradle/task…

f630b51

…s/worker/PreCommitHookFormatAction.kt Co-authored-by: Nicola Corti <[email protected]>

Update plugin-build/plugin/src/main/java/com/ncorti/ktfmt/gradle/task…

9c8a45a

…s/worker/PreCommitHookFormatAction.kt Co-authored-by: Nicola Corti <[email protected]>

Apply code review suggestions

dacd149

Specify files to be included using a gradle property

9466c7e

gpopides force-pushed the main branch from 83efe76 to 9466c7e Compare March 23, 2024 20:15

rm misc.xml

8662794

cortinico reviewed Mar 23, 2024

View reviewed changes

cortinico reviewed Mar 26, 2024

View reviewed changes

Move includes files to Base task and revert formatting changes to readme

29143ca

simonhauck mentioned this pull request Jul 15, 2024

#290 plugin not compatible with gradle configuration cache #312

Merged

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gpopides commented Mar 19, 2024

cortinico commented Mar 19, 2024

gpopides commented Mar 19, 2024

cortinico commented Mar 21, 2024

gpopides commented Mar 21, 2024

cortinico Mar 23, 2024

cortinico Mar 23, 2024

gpopides Mar 23, 2024

cortinico Mar 23, 2024

gpopides Mar 23, 2024

cortinico Mar 23, 2024

cortinico Mar 23, 2024

gpopides Mar 23, 2024

cortinico Mar 23, 2024

gpopides Mar 23, 2024

cortinico Mar 23, 2024

cortinico Mar 23, 2024

gpopides Mar 23, 2024

cortinico left a comment

gpopides commented Mar 24, 2024

cortinico Mar 26, 2024

gpopides Mar 26, 2024

cortinico Mar 26, 2024

gpopides Mar 26, 2024

cortinico Mar 26, 2024

cortinico Mar 26, 2024

gpopides Mar 26, 2024

	abstract class CreateGitPreCommitHookTask : SourceTask() {
	abstract class CreateGitPreCommitHookTask : DefaultTask() {

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

Are you sure you want to change the base?

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

Conversation

gpopides commented Mar 19, 2024

🚀 Description

📄 Motivation and Context

🧪 How Has This Been Tested?

📦 Types of changes

✅ Checklist

cortinico commented Mar 19, 2024

gpopides commented Mar 19, 2024

cortinico commented Mar 21, 2024

gpopides commented Mar 21, 2024

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

cortinico left a comment

Choose a reason for hiding this comment

gpopides commented Mar 24, 2024

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