Skip to content

Conversation

@lsm5
Copy link
Member

@lsm5 lsm5 commented Nov 24, 2025

See individual commits.

Looks like gofumpt is enabled on all three subprojects but evidently
this diff wasn't handled prior.

Signed-off-by: Lokesh Mandvekar <lsm5@redhat.com>
@github-actions github-actions bot added storage Related to "storage" package common Related to "common" package image Related to "image" package labels Nov 24, 2025
podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Nov 24, 2025
@podmanbot
Copy link

✅ A new PR has been created in buildah to vendor these changes: containers/buildah#6532

@lsm5 lsm5 force-pushed the golangci-extension-gofumpt branch from 8e0a92f to d0867db Compare November 24, 2025 18:25
@lsm5 lsm5 changed the title root .golangci.yml with extensions + gofumpt changes gofumpt changes Nov 24, 2025
podmanbot pushed a commit to podmanbot/buildah that referenced this pull request Nov 24, 2025
@lsm5
Copy link
Member Author

lsm5 commented Nov 24, 2025

Do we have any policy on naked returns? This was the result of running gofumpt v0.9.1 locally, but there's been some back and forth between v0.9.0 and v0.9.2 apparently.

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

run gofumpt -w .

but there's been some back and forth between v0.9.0 and v0.9.2 apparently.

We have make fmt which uses exactly the golangci-lint version that is going to be run in CI; that should, hopefully, make it clear which version of gofumpt is relevant. (That doesn’t prevent anyone from being stricter than the golangci-lint-included version, but I think it’s unreasonable to ask PR submitters to be stricter than the automatically-ran lint.)

Do we have any policy on naked returns?

Nothing written down AFAIK. (My personal experience is that named return values, naked return, and Go’s shadowing of := within a nested block tend to result in non-obvious bugs that are avoidable by forbidding named return values; and that returning a struct with named fields scales and reads better than returning 5 values, even if named in the declaration. But that’s a personal aesthetic preference, others have different preferences and there’s a lot of code in the containers/ repo that uses named returns.)

@Luap99
Copy link
Member

Luap99 commented Nov 25, 2025

I don't like naked returns as well so I am in favour of merging this even if gofumpt doesn't enforce it with the current golangci-lint version.

If we all agree we don't like it maybe we should look into enabling such a check explicitly.
Overall I do agree we should stick to golangci-lint fmt as source of truth for formatting related changes.

@lsm5
Copy link
Member Author

lsm5 commented Nov 25, 2025

Opening for review (and maybe merge if enough agreement).

FWIW, I have neovim setup to automatically run gofumpt on every write, so that's what first caused this change when I was working on something else, so I decided to make this a separate PR.

Btw, wdyt about having a unified .golangci.yml in topdir and having things like build-tags mentioned in Makefiles instead wherever required? Is there a need for each library to have its own config?

@lsm5 lsm5 marked this pull request as ready for review November 25, 2025 18:41
@mtrmac
Copy link
Contributor

mtrmac commented Nov 25, 2025

Btw, wdyt about having a unified .golangci.yml in topdir and having things like build-tags mentioned in Makefiles instead wherever required? Is there a need for each library to have its own config?

I… personally don’t want to spend much time tinkering, and especially bike-shedding, the best lint settings. I’m not saying there are no improvements to be had, and maybe there are classes of bugs that could be eliminated, but there are way too many outstanding bugs and RFEs for me to spend fine-tuning lint rules, or dealing with, in the worst case, dozens of formatting changes or false positives for no immediate end-user-benefit.

Copy link
Contributor

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

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

LGTM.

Thanks!

@mtrmac mtrmac merged commit 32a0839 into containers:main Nov 25, 2025
25 checks passed
@lsm5 lsm5 deleted the golangci-extension-gofumpt branch November 25, 2025 20:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

common Related to "common" package image Related to "image" package storage Related to "storage" package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants