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

Use gofumpt to format code #2975

Merged
merged 1 commit into from Jun 2, 2021
Merged

Conversation

kolyshkin
Copy link
Contributor

gofumpt (https://mvdan.cc/gofumpt) is a fork of gofmt with stricter rules.

Brought to you by

git ls-files \*.go | grep -v ^vendor/ | xargs gofumpt -s -w

Looking at the diff, all these changes make sense.

Also, replace gofmt with gofumpt in golangci.yml.

gofumpt (mvdan.cc/gofumpt) is a fork of gofmt with stricter rules.

Brought to you by

	git ls-files \*.go | grep -v ^vendor/ | xargs gofumpt -s -w

Looking at the diff, all these changes make sense.

Also, replace gofmt with gofumpt in golangci.yml.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

Perhaps we can squeeze this and #2974 in before 1.0?

@@ -6,4 +6,4 @@ run:

linters:
enable:
- gofmt
- gofumpt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can gofmt pass after gofumpt? If so, can we run both? If gofumpt doesn't get updated in the future, then we can fall back to gofmt.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can gofmt pass after gofumpt?

Yes it can. gofumpt is a superset.

If so, can we run both?

Does not make sense to me, as they are 99% similar and gofumpt is doing its job. Frankly, gofmt is somewhat abandoned, see e.g. golang/go#6996.

If gofumpt doesn't get updated in the future.

Oh, you mean to sort of re-check gofumpt results with gofmt. For that, we need to run gofumpt first in "fixing" mode, then run gofmt on top of it, which is a separate job.

golangci-lint runs all checkers in parallel, and so if we just add both gofumpt and gofmt, we'll end up double amount of linter warnings.

Now, I hope that we should trust @mvdan's good track record and rely on his promise of "running gofmt after gofumpt should be a no-op".

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TL;DR: we don't want both gofumpt and gofmt as it will just double the amount of linter warnings.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, gofumpt output is designed to be a strict subset of gofmt output. If you see gofmt changing a program after gofumpt ran on it, or if gofumpt itself isn't stable when run twice in a row, that's a bug. There have been a few of those over the years, but they should be rare and relatively easy to fix when reported.

This comment was marked as spam.

@kolyshkin kolyshkin added this to the 1.0.0 milestone Jun 1, 2021
@kolyshkin
Copy link
Contributor Author

Tentatively set 1.0.0 milestone as this seems like a good fit.

Copy link
Member

@cyphar cyphar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. It is interesting that so many of these were allowed.

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

Successfully merging this pull request may close these issues.

None yet

6 participants