-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
Include formatting check to the make test
(and thus also check
) rule
#1366
Conversation
fmt:
- find . -name "*.go" -type f -not -path "./vendor/*" | xargs gofmt -s -w
+ find . -name "*.go" -type f -not -path "./vendor/*" | xargs gofmt -d -s -w display diffs on drone logs. |
@appleboy I don't understand your comment. This PR is about making drone fail unless the PR code was threated by running |
@appleboy the way I read the docs for
|
Makefile
Outdated
.PHONY: test | ||
test: | ||
test: fmt-check |
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.
This make it part of make test
, not make check
as the title says. Change one of them 🙂
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.
clarified the title (check is an alias for test)
@strk I mean that the |
make check
rulemake test
(and thus also check
) rule
@appleboy gofmt -d seems indeed useful, but doesn't tell in the exit code if there was or not a diff. |
@strk Good work. It is working for me https://github.com/go-training/training/tree/master/example01 LGTM Don't forget to run |
@bkcsoft please lift your barrier and let this flow ! :) |
I've just installed Go 1.8 and |
Actually I just tried the |
I don't like such complicated make tasks without a bigger benefit. |
I don't like the complexity either, so if we cannot ensure any Go version would format the code in the same way, let's forget about it (and beware of the "make fmt" rule too as it might be doing and undoing the same thing) |
Any news on that ? |
If I'm not wrong I was using the same version used on Drone, still
the format was different, so maybe it's not just version but also
architecture ?
|
@strk How about the following check script so we can run fmt check with unstaged changes. GOFILES := find . -name "*.go" -type f -not -path "./vendor/*"
GOFMT ?= gofmt "-s"
.PHONY: fmt-check
fmt-check:
# get all go files and run go fmt on them
@files=$$($(GOFILES) | xargs $(GOFMT) -l); if [ -n "$$files" ]; then \
echo "Please run 'make fmt' and commit the result:"; \
echo "$${files}"; \
exit 1; \
fi; |
The reason to forbid running with unstaged changes is to avoid
changing the tree state. The current `fmt-check` rule does
a `checkout .` after the gofmt run, so you're left with what you
had before you started.
If gofmt -l ensures that, I'm ok with the change.
But where are GOFILES and GOFMT defined ?
Was your code just an example @appleboy ?
|
@strk I updated the previous comment and add missing variables on Makefile. I can confirm that |
@strk any updates? |
@appleboy I gave up due to the differences in formatting choices on different machines (mine, and drone's). Do you want to try from your side ? It'd be a pain to be unable to fix a problem reported by Drone because the developers |
Anyway, rebased for a final review. Running |
@strk We should ignore the |
LGTM let merge this if CI don't complain ^^ |
https://drone.gitea.io/go-gitea/gitea/3383 @strk Can you fix some format error? |
@appleboy as I wrote before, |
... and give it its own standalone target too (make fmt-check) Show diff on fmt-check failure Do not allow running "fmt-check" with incompatible go version Also simplify the `fmt` rule
also remove duplicated variable assignment for GOFILES
Ok with the rebase I do have format changed on |
@strk You are right. |
So now Drone was happy before my |
@strk So let's merge this PR or any concern? |
Uhm, as it's still happy (and I am too on my side) - let's merge this. |
@lunny Done. move to |
... and give it its own standalone target too (make fmt-check)
\cc @lunny, @tboerger, @bkcsoft