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

Proposal: Allow go fmt to return non-zero if code violations are found #41189

Closed
arvenil opened this issue Sep 2, 2020 · 7 comments
Closed

Proposal: Allow go fmt to return non-zero if code violations are found #41189

arvenil opened this issue Sep 2, 2020 · 7 comments
Labels
Milestone

Comments

@arvenil
Copy link

@arvenil arvenil commented Sep 2, 2020

Allow gofmt to return non-zero if code violations are found.

What version of Go are you using (go version)?

$ go version
go version go1.14.4 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What did you do?

test -z $(go fmt ./...)
go: downloading github.com/labstack/gommon v0.3.0
go: downloading github.com/labstack/echo/v4 v4.1.17

I know this topic was rejected several times (#24427, #24230) but things changed since last time and go fmt downloads now dependencies (#31976, #30577) and prints them to stderr breaking above hack if go fmt ./... is wrapped in another script e.g. test -z $(make -s fmt). This is really annoying. Also the trick hides the output. I understand sometimes it's trivial to use other tools to bypass the issue but this gets more and more complicated and we have no guarantee it won't change in the future.

I'm trying right now to have, both, status code = 1 and output and I'm running out of ideas. I also don't like the idea of filtering stderr from go fmt output to get rid of those "go: downloading". Not to mention that most of the people not familiar with shell cringe when they see test -z.

There is clearly need for fmt to exit with status code=1 (an option) and return info which files got changed to verify if project is correctly formatted.

Is there any chance to have another look at this and simplify our lives?

@gopherbot gopherbot added this to the Proposal milestone Sep 2, 2020
@gopherbot gopherbot added the Proposal label Sep 2, 2020
@mvdan
Copy link
Member

@mvdan mvdan commented Sep 2, 2020

I personally agree. The test -z trick is also unfortunate, because it requires a POSIX shell, so it makes it hard to share the same "check formatting" rule between Linux and Windows CI environments, for example.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 2, 2020

Apologies for the extra output.
After #40728, go fmt will stop printing those downloading messages.
You can use gofmt -l -w . instead of go fmt ./... for now.

I don't believe the exit code should change based on whether files were reformatted.
Reformatting files is expected behavior and should be considered success.
This should probably be closed as a duplicate of the other issues.

@arvenil
Copy link
Author

@arvenil arvenil commented Sep 2, 2020

Hi @rsc, there is no doubt that reformatting files is expected behavior and it should return success. I don't think that should be changed.

However, from my experience and from what I read, it's a common practice to use go fmt on CI to check if code in PR is formatted. Verifying if code is formatted is another behavior and as so would be nice to get a flag for it e.g. go fmt -verify.

I understand that maybe more proper behavior would be to add go fmt as a git hook so the code is always formatted before it is even pushed, but that's in a perfect world.

I just tried below but on CI there is no /dev/tty, I'm out of ideas.

go fmt ./... | tee /dev/tty | xargs -n 1 test -z
tee: /dev/tty: No such device or address

Feel free to close the issue, just wanted to share the pain and point out to new issue with printing output.

@rsc
Copy link
Contributor

@rsc rsc commented Sep 2, 2020

A few comments related to CI systems.

  1. The exact output of gofmt does change over time. If your CI system is being picky about the files being formatted, you may end up with skew between what version of gofmt the developer is using and what the CI system is using. That could be a problem. You should at least be aware of that.

  2. Running go fmt ./... is significantly more work than gofmt -w -l .. If you want gofmt, you should probably just run gofmt. That will solve the problem too. Also you can use gofmt -l . (dropping -w) to not write the files out; go fmt has no such option.

  3. Because of (1), at the very least any per-PR check should really apply only to the files being modified in that PR. If you build a list of those files, then you can pass them to gofmt -l. Another reason for (2).

I am not sure what you were trying to do with the tee /dev/tty. Did you try gofmt -l?

@arvenil arvenil changed the title Proposal: Allow gofmt to return non-zero if code violations are found Proposal: Allow go fmt to return non-zero if code violations are found Sep 2, 2020
@arvenil arvenil closed this Sep 2, 2020
@wchargin
Copy link

@wchargin wchargin commented Sep 8, 2020

  1. The exact output of gofmt does change over time

In other language ecosystems, one common solution to (1)/(3) is to pin a
version of the formatter and strictly enforce the formatting. Then, when
you decide to upgrade the formatter version, you can perform a blanket
reformat and instruct your source control system to ignore the
pure-reformat commit. That way, you don’t lose all the history or
pollute the blame. Git supports this with a blame.ignoreRevsFile:

Thus, you have black --check (Python), cargo fmt --check (Rust),
prettier --check (JavaScript, et al.).

(Personally, I’ve worked with all these ecosystems, and (1) and (3) have
not posed any issues for me, even across formatter version changes.)

Is there a reason that a solution like this is less attractive for Go?

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Sep 8, 2020

@wchargin What you described seems to be a popular solution used by many Go projects (aside from the blame.ignoreRevsFile feature, I haven't seen that before). There's more than one way to implement this approach in Go, and individual projects tend to have small differences in implementation details.

@wchargin
Copy link

@wchargin wchargin commented Sep 8, 2020

@dmitshur: Good to know; thanks. I’ll go with that for my project, then,
using go fmt ./... | diff -u /dev/null -, though go fmt -check like
all the other ecosystems would still be a welcome addition. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.