Skip to content

Conversation

@pkubowicz
Copy link

internalKtlintGitFilter is part of the cache key, and it changes with every commit, so getting a cache hit is extremely unlikely.

If you execute the commit hook (in the form before my changes) with Configuration Cache enabled, you will see:

Calculating task graph as configuration cache cannot be reused because the set of Gradle properties has changed: the value of 'internalKtlintGitFilter' was changed.

@JLLeitschuh
Copy link
Owner

I'm guessing this is going to fail CI because the PR doesn't update the test testing the the pre-commit script that is generated. Just and FYI.

Thanks for the contribution!

@JLLeitschuh
Copy link
Owner

Also, this needs a change log entry please!

internalKtlintGitFilter is part of the cache key,
and it changes with every commit, so getting a cache hit
is extremely unlikely.
@pkubowicz
Copy link
Author

I added a change log entry.

I also executed GitHookTasksTest and it passed. From what I see inside the test, it does not check the contents character by character, but rather asserts that certain key words (like the task name) appear. Also, the string --quiet only appears in production code, never in test sources.

@pkubowicz
Copy link
Author

Something for the future: instead of relying on -P (which does not work well with configuration cache), git filter can be modeled as a CLI option: https://docs.gradle.org/current/userguide/custom_tasks.html#sec:declaring_and_using_command_line_options

So instead of running:

./gradlew --quiet ktlintCheck -PinternalKtlintGitFilter="my-file"

the call will look like this:

./gradlew --quiet ktlintCheck --git-filter="my-file"

with the default for option not passed being **/*.kts,**/*.kt. This would require moving fun BaseKtLintCheckTask.applyGitFilter() away from GitHook.kt (this does not look like the responsibility of the hook code anyway) and closer to Task.action.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants