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

pre-commit hook config #188

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jessp01
Copy link
Contributor

@jessp01 jessp01 commented Aug 23, 2023

Added .pre-commit-config.yaml to the repo. To use it:

  • pip install pre-commit
  • Install the following Go packages:
  • In your clone, run: pre-commit install

This will create .git/hooks/pre-commit and following that, these tests will run every time you invoke git commit:

go fmt...................................................................Passed
go imports...............................................................Passed
go vet...................................................................Passed
go lint..................................................................Passed
go-critic................................................................Passed
shellcheck...............................................................Passed

added `.pre-commit-config.yaml` to the repo. To use it:

- `pip install pre-commit`
- Install the following Go packages:
  * go install github.com/go-critic/go-critic/cmd/gocritic@latest
  * go install https://pkg.go.dev/golang.org/x/tools/cmd/goimports@latest
  * go install golang.org/x/lint/golint@latest
- In your clone, run: `pre-commit install`

This will create .git/hooks/pre-commit and following that, these tests will run every time you invoke `git commit`:
```sh
go fmt...................................................................Passed
go imports...............................................................Passed
go vet...................................................................Passed
go lint..................................................................Passed
go-critic................................................................Passed
```
@jessp01
Copy link
Contributor Author

jessp01 commented Feb 23, 2024

Hello,

Just wondering whether you'd like to merge this?

Cheers,

@bitfield
Copy link
Owner

Hey @jessp01, sorry I missed this when you opened it. Thanks for the PR!

Could you say a word or two about what extra value you intend for this to add to the repo? For example, gofmt, goimports, and go vet are already integrated with my editor, as is staticcheck. go-critic is run in a CI action, as is govulncheck. shellcheck is a good idea in general, but there are no shell scripts for it to check in this project.

@jessp01
Copy link
Contributor Author

jessp01 commented Feb 25, 2024

Hi @bitfield ,

This does not replace a proper CI, of course, since you can't enforce the use of these prehooks in the clone commits are made in; in other words: you need to trust the developers to install the hooks:)
In my projects, I actually invoke the pre-commit hooks during CI as well; that way, I get the exact same checks on both ends (clone side, before pushes are made to avoid running CI jobs that are doomed to fail and CI to block pushes in the event these things weren't checked at commit time). You can see an example in my template project here: https://github.com/jessp01/grt (it actually makes use of script to replace tokens, by the way:)

It's great to use an IDE that integrates plugins that check for formatting/linting and import issues but, you can't be sure that all contributors to the project do the same and I do think there's value in offering ready-made prehooks that check for such issues and blocks commits that introduce them, regardless of the IDE used.

@bitfield
Copy link
Owner

Yes, I see your point that a pre-commit hook is opt-in, and that if you want to actually enforce things like gofmt, then it has to be done at the CI level. Would you like to update your PR to include adding these checks to the existing CI actions, too?

@jessp01
Copy link
Contributor Author

jessp01 commented Feb 25, 2024

Hi @bitfield ,

I've added invocations of the pre-commit hooks (the term "pre-commit hooks" obviously does not apply at this stage but you get my drift) in the CI (and changed the stage name accordingly so it reflects them). I've also added usage info in the README.

Let me know what you think?

Cheers,

@bitfield
Copy link
Owner

I think there's something I'm not quite understanding here: if you want to run, for example, gofmt in a CI action, wouldn't it be better to run it directly rather than installing a tool like pre-commit to run it for you?

I guess I'm not seeing what value pre-commit itself is adding here.

@jessp01
Copy link
Contributor Author

jessp01 commented Feb 26, 2024

Hi @bitfield ,

You wrote:

Would you like to update your PR to include adding these checks to the existing CI actions, too?

From which, I understood that you wanted me to add these checks to the CI:)
The only advantage of using the pre-commit util to run these tests in both CI and pre-commit hooks context is that you only have to maintain one set of checks, which you can then use wherever they are relevant. So, for instance, if you wanted to add another check, you'd only have to edit .pre-commit-config.yaml rather than both it and .github/workflows/test.yml.
If you prefer to invoke gocritic, gofmt, go vet, golint, etc directly in .github/workflows/test.yml, I can revert the changes I made to it.

@bitfield
Copy link
Owner

Yes, I see, sorry—that's on me for wording my suggestion too vaguely. By "these checks" I meant formatting, linting, and so on. As I say, I think running them directly, rather than via pre-commit, is the simplest and most direct way to get these steps into CI.

What would you think about using gofumpt instead of vanilla gofmt?

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