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 git hook version #474

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

KotlinIsland
Copy link
Contributor

@KotlinIsland KotlinIsland commented Apr 28, 2021

When git hook is updated users won't know. This change introduces a version number into the hook, and if that version number doesn't match the expected value then the gradle task will fail.

There is also a issue with the formatting of the git hook which I've fixed up.

@KotlinIsland KotlinIsland force-pushed the git-hook-version branch 5 times, most recently from a459a58 to 80d7d01 Compare April 28, 2021 02:09
@KotlinIsland KotlinIsland marked this pull request as ready for review April 28, 2021 02:10
@KotlinIsland
Copy link
Contributor Author

I'm not sure about the best approach for the version of the git hook, I would think matching the version of ktlint would be a bad idea as it would force users to update the hook when there are no changes.

Currently this requires manually updating the version when changes are made to GitHook.kt

Perhaps the best way to solve this would be to have a build step that checks for changes to GitHook.kt or generates the script and checks diff. If there are changes then increment the hookVersion.

@KotlinIsland KotlinIsland force-pushed the git-hook-version branch 3 times, most recently from d0a1490 to f6f315b Compare April 28, 2021 05:27
Comment on lines 93 to 96
// if FILTER_INCLUDE_PROPERTY_NAME exists then we are invoked from a git hook, check hook version
if (project.findProperty(hookVersion) != hookVersion) {
throw GradleException("Your ktlint git hook is outdated, please update by running the addKtlint*GitPreCommitHook Gradle task.")
}
Copy link
Owner

Choose a reason for hiding this comment

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

I'd be inclined to make this a warning instead of an error. Or, if we do want to make this an error, you should be able to disable it to make it a warning instead.

@Tapchicoma thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've changed this to be a warning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it's a warning then the user won't see it because --quiet.

Comment on lines +274 to +283
@Test
internal fun `Git hook should send the hook version to gradle`() {
projectRoot.setupGradleProject()
val gitDir = projectRoot.initGit()

build(":$INSTALL_GIT_HOOK_FORMAT_TASK").run {
assertThat(task(":$INSTALL_GIT_HOOK_FORMAT_TASK")?.outcome).isEqualTo(TaskOutcome.SUCCESS)
assertThat(gitDir.preCommitGitHook().readText()).contains("""-phookVersion=$hookVersion""")
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Could you update this test to use the @CommonTest format above?

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.

3 participants