Skip to content

Use gofumpt if available, and enable gofumpt linter - #3798

Merged
thaJeztah merged 2 commits into
docker:masterfrom
thaJeztah:gofumpt_linting
Nov 4, 2022
Merged

Use gofumpt if available, and enable gofumpt linter#3798
thaJeztah merged 2 commits into
docker:masterfrom
thaJeztah:gofumpt_linting

Conversation

@thaJeztah

@thaJeztahthaJeztah commented Sep 30, 2022

Copy link
Copy Markdown
Member

gofumpt (https://github.com/mvdan/gofumpt) provides a supserset of gofmt / go fmt, but not every developer may have
it installed, so for situations where it's not available, fall back to gofmt.

As our code has been formatted with gofumpt already, in most cases contributions
will follow those formatting rules, but in some cases there may be a difference,
which would already be flagged by manual code review, but let's also enable the
gofumpt linter.

With this change, make fmt will use gofumpt is available; gofumpt has been
added to the dev-container, so make -f docker.Makefile fmt will always use it.

@thaJeztahthaJeztah mentioned this pull request Sep 30, 2022
@thaJeztah
thaJeztahforce-pushed the gofumpt_linting branch 2 times, most recently from 380d7fc to a92c219CompareSeptember 30, 2022 11:51
@thaJeztah
thaJeztah marked this pull request as ready for review September 30, 2022 11:51
@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@crazy-max@vvoland ptal 😄

Comment threadMakefile Outdated
go list -f {{.Dir}} ./... | xargs gofmt -w -s -d
fmt: ## run gofumpt (if present) or gofmt
@if command -v gofumpt > /dev/null; then \
gofumpt -w -d . ; \

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.

gofumpt automatically excludes vendor/ so no need to use go list

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Hm... interesting; somehow my gofumpt didn't format all files?

#17 87.07 e2e/plugin/basic/basic.go:17: File is not `gofumpt`-ed (gofumpt)
#17 87.07 if err := os.MkdirAll(p, 0755); err != nil {
#17 87.07 cli/context/store/metadatastore.go:31: File is not `gofumpt`-ed (gofumpt)
#17 87.07 if err := os.MkdirAll(contextDir, 0755); err != nil {
#17 87.07 cli/context/store/metadatastore.go:38: File is not `gofumpt`-ed (gofumpt)
#17 87.07 return os.WriteFile(filepath.Join(contextDir, metaFile), bytes, 0644)
#17 87.07 cli/context/store/store.go:249: File is not `gofumpt`-ed (gofumpt)

Looks like the linter uses an explicit -lang, which (for go1.19)
results in some additional formatting for octal values.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
gofumpt provides a supserset of gofmt / go fmt, but not every developer may have
it installed, so for situations where it's not available, fall back to gofmt.
As our code has been formatted with gofumpt already, in most cases contributions
will follow those formatting rules, but in some cases there may be a difference,
which would already be flagged by manual code review, but let's also enable the
gofumpt linter.
With this change, `make fmt` will use gofumpt is available; gofumpt has been
added to the dev-container, so `make -f docker.Makefile fmt` will always use it.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@codecov-commenter

codecov-commenter commented Sep 30, 2022

Copy link
Copy Markdown

Codecov Report

Merging #3798 (c2f1671) into master (65438e0) will not change coverage.
The diff coverage is 66.66%.

Additional details and impacted files
@@ Coverage Diff @@## master #3798 +/- ##
=======================================
Coverage 59.21% 59.21% =======================================
Files 288 288 Lines 24605 24605 =======================================
Hits 14571 14571 Misses 9159 9159 Partials 875 875 

Comment threadMakefile
go list -f {{.Dir}} ./... | xargs gofmt -w -s -d
fmt: ## run gofumpt (if present) or gofmt
@if command -v gofumpt > /dev/null; then \
gofumpt -w -d -lang=1.19 . ; \

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.

Looks like I need to add an explicit -lang=1.19, otherwise it doesn't pick up the go version for which to format. Probably because we don't have a go.mod

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

@crazy-max ptal 🤗

@crazy-maxcrazy-max left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM if opt-in.

Do we want a scheduled worklow to fix-up and open PR as semi-automation process?

@thaJeztah

Copy link
Copy Markdown
MemberAuthor

Let me bring this one in 👍

@thaJeztah
thaJeztah merged commit 8a19043 into docker:masterNov 4, 2022
@thaJeztah
thaJeztah deleted the gofumpt_linting branch November 4, 2022 18:04
@thaJeztahthaJeztah added this to the 23.0.0 milestone May 3, 2023
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@crazy-max@apqjd