Use gofumpt #2500

kolyshkin · 2024-08-02T23:25:13Z

Makefile,hack/validate: rm gofmt, add gofmt linter to golangci-lint
Gofumpt everything

gofumpt is essentially a stricter gofmt, adding some extra formatting
rules. To me, using it almost always result in better looking and more
readable code.

This commit does two things:

formats all sources with gofumpt -w (using v0.6.0);
replaces gofmt linter with gofumpt (since it is a superset).

PS If you already use gopls to autoformat the sources from your $EDITOR,
you can switch to gofumpt; see 1.

mtrmac

Thanks!

I don’t feel too strongly about this: The code does look a bit better (notably the handling of empty lines), but the cost is an extra step and frustration for occasional contributors, who are not specifically set up for c/image, file a PR, and see it fail tests. On balance, I don’t think it’s worth it — but that’s a guess.

kolyshkin · 2024-08-05T18:10:18Z

the cost is an extra step and frustration for occasional contributors, who are not specifically set up for c/image, file a PR, and see it fail tests.

I understand the concern; yet, most contributors use gofmt already, and gofumpt is not a huge superset of gofmt, so in many cases code formatted by gofmt will pass gofumpt.

Instead, enable gofmt linter for golangci-lint. Same checking in CI, less infra code to maintain. Signed-off-by: Kir Kolyshkin <[email protected]>

gofumpt is essentially a stricter gofmt, adding some extra formatting rules. To me, using it almost always result in better looking and more readable code. This commit does two things: - formats all sources with gofumpt -w (using v0.6.0); - replaces gofmt linter with gofumpt (since it is a superset). PS If you already use gopls to autoformat the sources from your $EDITOR, you can switch to gofumpt; see [1]. [1]: https://github.com/golang/tools/blob/master/gopls/doc/settings.md#gofumpt-bool Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin · 2024-08-10T02:52:00Z

rebased

kolyshkin · 2024-08-15T08:53:10Z

@mtrmac what do you want me to do about this PR? Options I see are:

close it (and never mention gofumpt again 😃)
remove the second commit (i.e. only add gofmt)
modify the second commit to not add gofumpt to golangci-lint (but keep gofumpt'ed code)
same as above, but make gofumpt linter optional (so any warnings from it won't prevent a PR from being merged)

mtrmac · 2024-08-15T19:24:19Z

copy/manifest_test.go

@@ -126,7 +126,8 @@ func TestDetermineManifestConversion(t *testing.T) {
 		},
 		// Conversion necessary, a preferred format is not acceptable
 		{
-			"s2→OCI", manifest.DockerV2Schema2MediaType, []string{v1.MediaTypeImageManifest},
+			"s2→OCI", manifest.DockerV2Schema2MediaType,
+			[]string{v1.MediaTypeImageManifest},


In this file: The line breaks are currently consistent across the table… and shorter entries, even if wider help scanning.

mtrmac · 2024-08-15T19:25:06Z

docker/daemon/client_test.go

@@ -69,7 +69,7 @@ func TestTlsConfigFromCertPath(t *testing.T) {
 }

 func TestSkipTLSVerifyOnly(t *testing.T) {
-	//testDir := testDir(t)
+	// testDir := testDir(t)


I think this comment can just be dropped.

mtrmac · 2024-08-15T19:26:53Z

docker/internal/tarfile/writer.go

@@ -340,21 +340,26 @@ type tarFI struct {
 func (t *tarFI) Name() string {
 	return t.path
 }
+


Absolutely non-blocking: I’m not in love with these added lines. But then I can see argument that tarFI could be in a separate file instead…

Perhaps this can be reformatted like this instead?

func (t *tarFI) Name() string { return t.path } func (t *tarFI) Size() int64 { return t.size } func (t *tarFI) Mode() os.FileMode { if t.isSymlink { return os.ModeSymlink } return 0o444 } func (t *tarFI) ModTime() time.Time { return time.Unix(0, 0) } func (t *tarFI) IsDir() bool { return false } func (t *tarFI) Sys() any { return nil }

*shrug* That also works… I realize this is not worth that much discussion.

mtrmac · 2024-08-15T19:38:34Z

internal/image/docker_schema2_test.go

@@ -373,30 +374,39 @@ type memoryImageDest struct {
 func (d *memoryImageDest) Reference() types.ImageReference {
 	return refImageReferenceMock{ref: d.ref}
 }
+


Non-blocking: Aesthetically I like these mocks without the blank lines, but, meh, not that important. Alternatively, this could be moved, maybe as a Forbidden* parent, to internal/testing/mocks. (It currently isn’t there because there is only a single user.)

mtrmac · 2024-08-15T19:41:38Z

manifest/common_test.go

+		{"unknown", nil, ""},
+		{"unknown", &compression.Gzip, ""},
+		{"", nil, ""},
+		{"", &compression.Gzip, ""},
 	} {


These are currently organized one line per input “MIME type”, e.g. all for AU on a single line.

I suppose the test could be restructured to make that an explicit data structure instead; with extra type names that might end up with more noise, I’m not sure.

mtrmac · 2024-08-15T19:44:00Z

pkg/blobinfocache/internal/test/test.go

+			{d: digestCompressedA, lr: "A1"},
+			{d: digestCompressedA, lr: "A2"},
+			{d: digestCompressedB, lr: "B1"},
+			{d: digestCompressedB, lr: "B2"},


Throughout this file, the two items on a single line show the relationship — same digest, specific order of locations.

I suppose the test could be restructured to make that an explicit data structure instead; with extra type names that might end up with more noise, I’m not sure.

mtrmac · 2024-08-15T19:47:28Z

storage/storage_transport_test.go

-			root + "suffix4"}, // A valid root@graph+run set
-		{"[" + driver + "@" + root + "suffix3+" + root + "suffix4:options,options,options]",
+			root + "suffix4",
+		}, // A valid root@graph+run set


Really the comments would be more helpful be on the { line of the test

mtrmac · 2024-08-15T19:51:41Z

I’d personally prefer not to add the gofumpt format enforcing, so, if it is up to me, not add that part. I’m open to being overridden on this, e.g. if all the projects standardized on that.

(The “more general principle” is that I don’t really want to be in the business of fine-tuning the linter set without a specific reason to do so. As c/common or Consider allowing more linters storage#1579 shows, it can result in a long list of linters that is created in a short time period when someone is interested, and then that list may easily end up not really carefully maintained over time, and on net much worse than just keeping the defaults.)
The gofumpt’ed code changes are mostly nice to have, why not. In some cases, e. g. tests, the formatting is somewhat semantically relevant … so that would be more work. I’m open to that if you are — commented on the relevant cases.
I don’t know about the separate gofmt binary vs. the one inlined in golangci-lint. The gofmt output does change once in a few years… but then users running things locally can have either golangci-lint or Go itself in a different version than the project’s CI, so it’s hard to say which one is “more correct”.

WRT infra concerns, right now gofmt is built into our images, but golangci-lint is installed at run-time — so, in that sense, the gofmt binary is “more correct”, but obviously the right thing to do is to install golangci-lint in the images as well, and then the calculation changes.

… actually, as long as we have a make fmt target invoking the gofmt binary, doing the checks using the same binary seems clearly more correct to me. (I’m not at all sure we need that target… pragmatically most users probably trigger formatting by an IDE format-on-save feature. But the target exists right now.)
An optional linter… is a thought. That might be valuable for the developers of a new PR, but if a new code adding a warning is merged, wouldn’t that annoy everyone else working on that file (or even unrelated files) later? I think that would not be worth it. We want most linters to be enforced also on existing code, so that future stricter linters improve the whole codebase, but a warning-only linter would only need to report on the new changes. (We do have a precedent for that kind of behavior, in git-validation; I didn’t check whether golangci-lint can do that.)

kolyshkin · 2024-09-19T15:15:50Z

The main purpose of gofumpt is same as gofmt, i.e. "enable and forget", and if we can not do that, it doesn't make sense to have it.

mtrmac reviewed Aug 5, 2024

View reviewed changes

kolyshkin force-pushed the gofmt branch from ac32461 to 04865d9 Compare August 5, 2024 18:12

kolyshkin added 2 commits August 9, 2024 19:49

Makefile,hack/validate: rm gofmt

8850723

Instead, enable gofmt linter for golangci-lint. Same checking in CI, less infra code to maintain. Signed-off-by: Kir Kolyshkin <[email protected]>

kolyshkin force-pushed the gofmt branch from 04865d9 to 0e8197c Compare August 10, 2024 02:51

mtrmac reviewed Aug 15, 2024

View reviewed changes

kolyshkin closed this Sep 19, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use gofumpt #2500

Use gofumpt #2500

kolyshkin commented Aug 2, 2024

mtrmac left a comment

kolyshkin commented Aug 5, 2024

kolyshkin commented Aug 10, 2024

kolyshkin commented Aug 15, 2024

mtrmac Aug 15, 2024

mtrmac Aug 15, 2024

mtrmac Aug 15, 2024

kolyshkin Aug 16, 2024

mtrmac Aug 16, 2024

mtrmac Aug 15, 2024

mtrmac Aug 15, 2024

mtrmac Aug 15, 2024

mtrmac Aug 15, 2024

mtrmac commented Aug 15, 2024

kolyshkin commented Sep 19, 2024

Use gofumpt #2500

Use gofumpt #2500

Conversation

kolyshkin commented Aug 2, 2024

mtrmac left a comment

Choose a reason for hiding this comment

kolyshkin commented Aug 5, 2024

kolyshkin commented Aug 10, 2024

kolyshkin commented Aug 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mtrmac commented Aug 15, 2024

kolyshkin commented Sep 19, 2024