Use ignore directive to ignore test files not to be passes to gofumpt

[kyu08]

Motivation

To replace the workaround introduced in #4809 with ignore directive to make the code simpler.

Overview

This is a follow-up PR to #4809.
I have added 4 changes due to updating gofumpt to remove git ls-files workaround.

Please take a look at each commit messages for details.

Please check if the PR fulfills these requirements

• Cheatsheets are up-to-date (run go generate ./...)
• Code has been formatted (see here)
• Tests have been added/updated (see here for the integration test guide)
• Text is internationalised (see here)
• If a new UserConfig entry was added, make sure it can be hot-reloaded (see here)
• Docs have been updated if necessary
• You've read through your own file changes for silly mistakes etc

[kyu08]

This commit does not change actual behavior of this functions because err is returned originally because this function uses "named return value".

[stefanhaller]

It may seem like a good idea to pin gofumpt to a specific version, but here you only do it for make format (and, somewhat implicitly, for golangci-lint). I find it important to also make all other places that call gofumpt agree on that version. The most important one of these is format-on-save in VS Code, which currently relies on gopls to format the file, which (as far as I can tell) will simply call whatever version of gofumpt happens to be installed on the system. On my machine this is currently 0.4.0. (I don't remember having installed it, so I suppose it was auto-installed either by VS Code's go extension or by gopls, and then never updated.)

The only way I could find to pin format-on-save to a specific version is with a patch like this:

diff --git a/.vscode/settings.json b/.vscode/settings.json
index dd4398af90..ab5f329f2d 100644
--- a/.vscode/settings.json
+++ b/.vscode/settings.json
@@ -1,6 +1,5 @@
 {
   "gopls": {
-    "formatting.gofumpt": true,
     "ui.diagnostic.staticcheck": true,
     "ui.diagnostic.analyses": {
       // This list must match the one in .golangci.yml
@@ -24,6 +23,8 @@
   },
   "go.alternateTools": {
     "golangci-lint-v2": "${workspaceFolder}/scripts/golangci-lint-shim.sh",
+    "customFormatter": "${workspaceFolder}/scripts/gofumpt-shim.sh",
   },
   "go.lintTool": "golangci-lint-v2",
+  "go.formatTool": "custom",
 }
diff --git a/scripts/gofumpt-shim.sh b/scripts/gofumpt-shim.sh
new file mode 100755
index 0000000000..8087384093
--- /dev/null
+++ b/scripts/gofumpt-shim.sh
@@ -0,0 +1,8 @@
+#!/bin/sh
+
+set -e
+
+# Must be kept in sync with ...
+version="v0.9.1"
+
+go run "mvdan.cc/gofumpt@$version" "$@"

The downside is that it makes pressing Command-S slower (not very much, but still noticeable), so unless we find a better way to solve this, I'm against this change.

Other places that also call or install gofumpt are .devcontainer/Dockerfile and (maybe?) the newly added nix flake.

[kyu08]

Thanks for the feedback!

The downside is that it makes pressing Command-S slower (not very much, but still noticeable), so unless we find a better way to solve this, I'm against this change.

I see.

Then how about this?

• We will not incorporate the VSCode setting you suggested. So developers who are using VSCode may run outdated gofumpt.
• Add a message asking developers to "run make format or update the gofumpt binary, and then push the changes to pass CI" just in case format task fails on CI. And in that case, developers need to run make format or update and run gofumpt.

If we adopt this solution, Command+S won't become slow.
Of course the number of times CI fails may increase slightly, however I think it's a not bad compromise.

Other places that also call or install gofumpt are .devcontainer/Dockerfile and (maybe?) the newly added nix flake.

Thanks for the input.
I will fix them if we can agree on this change.

[stefanhaller]

Thinking about it more, you could also argue that we don't need to pin the version of gofumpt at all, and in fact I think that's what I would propose right now.

Really, the only reason why pinning came up now is because we want to ensure that people running make format use a new enough version that understands the ignore directive. I'm not bothered by this at all though; since this is only done locally, not on CI, you could argue that people who want to use make format (and I wouldn't be surprised if that's nobody except you 😄) simply need to install a new enough version; we don't have to enforce this. I would guess that most people use editor integration to format on save.

Another reason to pin the version might be the naked return thing; however, we don't really have to pin it for that, either. We just need to fix our code so that all versions of gofumpt are happy with it (which you did in this branch).

The only reason where we would need to pin the version is when different versions of gofumpt disagree on how a particular piece of code is formatted, in which case the code would oscillate back and forth as you switch between versions. I have never heard of an example of this happening, and if we ever run into such a case, we can react then.

I also find the whole topic not important enough to spend a lot of time on it, to be honest.

[kyu08]

Thanks for the detailed reply!

OK. So I'm going to close this PR for now.

I'll make a change when it needed in the future.

[stefanhaller]

You must have got me wrong, I wasn't asking you to close the PR. Some of the changes are still useful; for example, fixing the naked returns is a good change for people who already updated their gofumpt. I also don't have objections against removing the git ls-files workaround; I just argued against pinning the gofumpt version.

I cleaned up the commit history a bit, and dropped the golangci-lint update. I'm happy to merge the PR in this state; please have a look if you agree.

[kyu08]

I misunderstood that you don't want to spend any time on this PR, and you think changes should be made when they become necessary.

Anyway, thanks for the fix!
LGTM!