Update pre-commit git hook

celestiaorg / celestia-app

Celestia consensus node

https://celestiaorg.github.io/celestia-app/

Apache License 2.0

345 stars 285 forks source link

Update pre-commit git hook #674

Closed rootulp closed 2 years ago

rootulp commented 2 years ago

Context

https://github.com/celestiaorg/celestia-app/pull/672

Tasks

[x] Delete precommit file https://github.com/celestiaorg/celestia-app/pull/675
[ ] Update them to match linters enforced by CI
- Update script to run make fmt. Note: this command is slow (1 to 2 seconds) and would have negative effects on devx if the slowdown was experienced on every commit
[ ] Make githooks more discoverable
- Option A: Include a section in https://github.com/celestiaorg/celestia-app#contributing that links to the githooks README or describes how to install these hooks.
- Option B: Move githooks to ./githooks

rootulp commented 2 years ago

I just encountered a situation where the githook was not helpful. I had changes in go.mod that I did not intend to include in a commit (temporary replace to use local celestia-core). However, this line added them:

https://github.com/celestiaorg/celestia-app/blob/main/contrib/githooks/pre-commit#L41

so I'm disabling the githook on my machine

evan-forbes commented 2 years ago

fwiw I'll probably disable it too, as I also do that very often

rootulp commented 2 years ago

@SweeXordious have you encountered the above scenario? Does that change your opinion on wanting to keep these githooks?

rach-id commented 2 years ago

@rootulp I didn't run them locally to be honest, but I guess they shouldn't be changing any files. For example, they should check if the file is gofumpted, however, shouldn't apply that themselves. I guess the following lines are making changes:

    gofmt -w -s $file
    misspell -w $file
    goimports -w -local github.com/celestiaorg/celestia-app $file
    git add $file

Maybe, if gofmt found that the file is not formatted, it should just return something different from 0, instead of fixing the issue.

rootulp commented 2 years ago

Maybe, if gofmt found that the file is not formatted, it should just return something different from 0, instead of fixing the issue.

This is possible and according to these docs would abort the commit. However, I think it's often useful to be able to commit things that would violate go mod tidy or linters b/c the work is in progress.

I didn't run them locally

If the current githooks aren't helpful to us, I'll re-propose deleting them b/c currently they are a maintenance burden with no benefit to core contributors of this repo. Any objections @SweeXordious @evan-forbes ?

evan-forbes commented 2 years ago

no moar git hooks thumbsdown