-
Notifications
You must be signed in to change notification settings - Fork 879
Adds pre-commit hook, hook config script, and relevant README #856
Conversation
cd $REPO_ROOT | ||
|
||
GOFMT_CHANGES=$(gofmt -s -l -w . 2>&1) | ||
if [ -n "$GOFMT_CHANGES" ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason to deviate from the circleci syntax? (eg: maybe it would make maintenance easier down the road if they were "closer"?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For one, we can actually run gofmt
in a way that writes its updates in place pre-commit. Secondly, I want to make sure it's clear to a user why their commit was rejected, which is a lot less obvious without all of the chrome of a CI web interface. We can also port all this syntax over to the circle.yml, but it's not very conducive for complex commands like multiline if-statements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good enough! Thanks for clarifying Brian. LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will this place the gofmt
changes in the index and proceed with the commit? I would prefer that it fails and the results are uncommitted. Also, I am not sure about gating commits on golint
. Looking here, it seems like the intent is to make golint
quite noisy. This will be problematic when exported APIs migrate from wishes of golint
maintainers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with your concerns about golint
, but we're already using it as a gate in our CI setup, so this is no more restrictive. I can also change the gofmt
semantics to just fail and complain if you prefer that behavior.
Actually, let me push a fix for this script before you merge this. Just found an edge case. |
Alright, I tested this most recent version with various |
The pre-commit hook will automatically gofmt code in place, warning you about any changes. It will also fail to commit if either golint or go vet fails.
Does it give you a change to review the changes before adding them to the commit? |
I've removed the auto-fmt behavior in my most recent commit. |
LGTM! |
Adds pre-commit hook, hook config script, and relevant README
The pre-commit hook will automatically gofmt code in place, warning you about any changes. It will also fail to commit if either golint or go vet fails.
As git hooks cannot be added to a repository in a way that automatically applies them,
configure-hooks.sh
can be ran to link this pre-commit hook.@dmp42 @stevvooe