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

Lock the version of gofmt or stop using it as a lint check #459

Closed
Hexcles opened this issue Aug 20, 2018 · 6 comments
Closed

Lock the version of gofmt or stop using it as a lint check #459

Hexcles opened this issue Aug 20, 2018 · 6 comments

Comments

@Hexcles
Copy link
Member

Hexcles commented Aug 20, 2018

Today when working on #439 , I found the gofmt on my machine and Travis disagreed with each other. After a bit of research, it turns out this is pretty much WAI:

https://tip.golang.org/pkg/go/format/

Note that formatting of Go source code changes over time, so tools relying on consistent formatting should execute a specific version of the gofmt binary instead of using this package. That way, the formatting will be stable, and the tools won't need to be recompiled each time gofmt changes.

For example, pre-submit checks that use this package directly would behave differently depending on what Go version each developer uses, causing the check to be inherently fragile.

And by the way, this warning is only added recently (Go 1.11, which hasn't been released yet). golang/go#26228

@Hexcles
Copy link
Member Author

Hexcles commented Aug 20, 2018

@mdittmer @lukebjerring @foolip please vote.

I vote for dropping gofmt from presubmit checks (I don't want to vendor such a basic tool in the project and I trust everyone using gofmt automatically in their editors).

@foolip
Copy link
Member

foolip commented Aug 20, 2018

It could well happen that I send a PR that makes some string longer that I never even try to compile or run, or reformat.

Best I think would be if the Travis job just pushed a commit to fix the problem, then it doesn't matter if versions disagree. This would require a little bit of work though, and would change the PR being tested. Worst case would be infinite reformat loop due to some bug :)

@lukebjerring
Copy link
Contributor

We can drop gofmt. vscode does it automatically anyway, and most (if not all) of us use that.

@lukebjerring
Copy link
Contributor

Fixed by #558

@Hexcles
Copy link
Member Author

Hexcles commented Sep 27, 2018

As it turns out, locking the version of gofmt isn't enough. gofmt by itself is mostly a wrapper. The work is done by go parser and printer, which are core libraries. That means, if we really want to do it, we need to lock the version of go.

I support dropping gofmt altogether from our lints. @lukebjerring @mdittmer ?

@mdittmer
Copy link
Contributor

I approve

Let's drop gofmt check.

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

No branches or pull requests

4 participants