Run checks in pre-commit git hook

[andrewgordstewart]

Currently, contributing.md includes two instructions:

1. format your code using gofmt -w .
2. lint your code using golangci-lint run

Making these the dev's responsibility

1. adds mental overhead to ones development flow
2. prevents the dev from learning about mistakes until pushed to GH

These checks could run in a pre-commit hook, reducing mental overhead, speeding up time-to-learn-issues and cleaning up the commit history.

(2) leads to a noisier commit history. For instance, see c2b2d64 and 75981e9 from #146 and 272f1b9 and 83c2560 from #147.

[andrewgordstewart]

147 is particularly illuminating: revealing the two separate issues required two sequential pushes to GitHub.

[NiloCK]

I am in favour, having routinely been "caught" specifically by golangci-lint run warnings. VSCode seems to do well for keeping gofmt -w up to date on save, but for whatever reason it doesn't seem to report exactly the same set of linting issues produced by the command line run.

I'm not well acquainted with git hooks, but as far as I can tell they are defined inside .git/hooks, which means that they themselves are not themselves subject to version control, and (maybe) require some local setup for each dev?

[NiloCK]

From https://git-scm.com/book/en/v2/Customizing-Git-An-Example-Git-Enforced-Policy:

Because hooks aren’t transferred with a clone of a project, you must distribute these scripts some other way and then have your users copy them to their .git/hooks directory and make them executable.

Which seems pretty darn annoying.

[geoknee]

Another source with suggested workflow: https://tutorialedge.net/golang/improving-go-workflow-with-git-hooks/

Perhaps this is something we simply let individual devs handle. We can provide example git hooks to make things easier for them to setup if desired? We could also provide a script to combine all of the checks into a single command 🤷 . I recall in the past that folks had different opinions about using git hooks anyway (although that was with a "slower" language, the objection being that the hook introduced an unacceptable delay into making a commit). So I suspect it may not be worth the effort required to set it all up.

[NiloCK]

This is related to #158.

I think we probably should try a hooks solution so that we can

• have a documented-by-code standard for applying the test suite
• avoid these persistent CI failures on pull requests

Suggestion:

• write a pre-push hook that only runs on push, which errors on non-zero runs of whatever combination of linting / formatting / testing we think is appropriate.
• include this in-repo (.hooks?)
• align it with gh actions
• (optional) add a post-merge hook with contents ~=s cp ./.hooks/pre-push ./.git/hooks/pre-push so that the local pre-push hook remains in sync with updates to the repo.

Devs will have to manually move the contents of ./.hooks to ./git/hooks once, and will not suffer any slow-down on individual local commits.

[geoknee]

I wasn't aware of a pre-push hook. That sounds much better to me. It's highly likely that I will want to commit some "bad" code from time to time. Much less likely that I will want to share that code by pushing it.

have a documented-by-code standard for applying the test suite

I agree this is important -- but doesn't the existing github action do that? The git hook might get out of sync with the github action?

avoid these persistent CI failures on pull requests

This is nice to avoid if we can. Speaking entirely personally, it hasn't caused me too much hassle, especially since the github action runs so quickly.

[andrewgordstewart]

I wasn't aware of a pre-push hook. That sounds much better to me. It's highly likely that I will want to commit some "bad" code from time to time. Much less likely that I will want to share that code by pushing it.

In some codebases, it is considered very valuable for the codebase to be in a good state on every commit. I'm not sure go-nitro is one of those code bases.

have a documented-by-code standard for applying the test suite

I agree this is important -- but doesn't the existing github action do that? The git hook might get out of sync with the github action?

It does, but it feels "disconnected" from contributing.md. How do I know that I should be reading .github/workflows/go.yml to understand the standards of this repo? (Linking to that file doesn't seem particularly friendly.)

The GH action and git hook could refer to a common script. In this way, a dev can be told "our standards are maintained by running go standardize, which runs bin/standardize.go. This script is run on each push to GitHub in a GH action."

[geoknee]

Closing, see #219 for rationale.