Skip to content

Makefile: use go1.22 semantics for gofumpt - #5684

Merged
thaJeztah merged 1 commit into
docker:masterfrom
thaJeztah:gofumpt_go_version
Dec 18, 2024
Merged

Makefile: use go1.22 semantics for gofumpt#5684
thaJeztah merged 1 commit into
docker:masterfrom
thaJeztah:gofumpt_go_version

Conversation

@thaJeztah

Copy link
Copy Markdown
Member

gofumpt defaults to using the go version from go.mod, but as we don't have one, we need to set it explicitly.

- A picture of a cute animal (not mandatory but encouraged)

gofumpt defaults to using the go version from go.mod, but as we don't
have one, we need to set it explicitly.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@codecov-commenter

codecov-commenter commented Dec 11, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.50%. Comparing base (26526f9) to head (b8fd205).
Report is 26 commits behind head on master.

Additional details and impacted files
@@ Coverage Diff @@## master #5684 +/- ##
==========================================
- Coverage 59.53% 59.50% -0.03% 
==========================================
Files 346 346 Lines 29371 29371 ==========================================
- Hits 17486 17478 -8 - Misses 10914 10923 +9 + Partials 971 970 -1 

Comment threadMakefile
fmt: ## run gofumpt (if present) or gofmt
@if command -v gofumpt > /dev/null; then \
gofumpt -w -d -lang=1.21 . ; \
gofumpt -w -d -lang=1.23 . ; \

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: We could probably have a variable on the top of the file so it's more discoverable

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Hm. Yeah, I was considering a bit how we could improve this, and went a bit back-and-forth;

  • It's not really critical (switching did not result in a diff, so looks like there were no version-specific changes)
  • It should no longer be needed when we have a go.mod, so .. 🤞 should go away in the near future (so didn't want to add the extra code-churn)
  • but I think we should look if we can containerise this (add a docker bake target); that way we would remove the requirement to have gofumpt installed, and make sure that we run it in a consistent environment (and expected version of go)
  • ☝️ similar to how we do vendoring
  • Was actually asking @crazy-max Yesterday (i.e., should we have some templates for such things?); related to Gofumpt all the things compose#12392 (comment)

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Let me bring this one in, but if I have some time, I may have a look if we can containerise this (per above)

@thaJeztah
thaJeztah merged commit 2f67b2f into docker:masterDec 18, 2024
@thaJeztah
thaJeztah deleted the gofumpt_go_version branch December 18, 2024 12:41
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@thaJeztah@codecov-commenter@vvoland@laurazard