Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update golangci configuration #1785

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 73 additions & 15 deletions .golangci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,47 +16,105 @@ run:
deadline: 5m

linters:
fast: false
disable-all: true
enable:
#- bodyclose
# Enabled by Default
# - deadcode ! deprecated since v1.49.0; replaced by 'unused'
- errcheck
- gosimple
- govet
- ineffassign
- staticcheck
- typecheck
- unused
# - varcheck ! deprecated since v1.49.0; replaced by 'unused'
# Disabled by Default
#- asasalint
#- asciicheck
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't realize our .golangci.yml had abunch of rules commented out. That seems weird. And makes the linter file harder to read: I don't think anyone really needs to know what rules we explicitly aren't using, that's implied by them not being in the file to begin with.

Can we just remove them?

#- bidichk
#- bodyclose
#- containedctx
#- contextcheck
#- cyclop
#- decorder
#- depguard
#- dogsled
#- dupl
- errcheck
#- durationcheck
#- errchkjson
#- errname
#- errorlint
#- execinquery
#- exhaustive
#- exhaustivestruct !
#- exhaustruct
#- exportloopref
#- forbidigo
#- forcetypeassert
#- funlen
- gas
#- gci
#- gochecknoglobals
#- gochecknoinits
#- gocognit
- goconst
#- gocritic
#- gocyclo
#- gofmt
#- godot
#- godox
#- goerr113
- gofmt
#- gofumpt
#- goheader
- goimports
- golint
#- golint ! Use revive instead
#- gomnd
#- gomoddirectives
#- gomodguard
#- goprintffuncname
#- gosec
#- gosimple
- govet
- ineffassign
- interfacer
#- grouper
#- ifshort !
#- importas
- interfacer # !
#- ireturn
#- lll
- maligned
#- maintidx
#- makezero
#- maligned # Replaced by govet 'fieldalignment'
- megacheck
#- misspell
#- nakedret
#- nestif
#- nilerr
#- nilnil
#- nlreturn
#- noctx
#- nolintlint
#- nonamedreturns
#- nosnakecase !
#- nosprintfhostport
#- paralleltest
#- prealloc
#- predeclared
#- promlinter
- revive
#- rowserrcheck
#- scopelint
#- staticcheck
#- scopelint !
#- sqlclosecheck
#- structcheck ! deprecated since v1.49.0; replaced by 'unused'
#- stylecheck
#- typecheck
#- tagliatelle
#- tenv
#- testpackage
#- thelper
#- tparallel
- unconvert
#- unparam
- unused
# - varcheck ! deprecated since v1.49.0; replaced by 'unused'
#- usestdlibvars
#- varnamelen
#- wastedassign
#- whitespace
fast: false
#- wrapcheck
#- wsl
20 changes: 8 additions & 12 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,15 @@ ifeq (, $(shell which golangci-lint))
$(warning "could not find golangci-lint in $(PATH), run: curl -sfL https://install.goreleaser.com/github.com/golangci/golangci-lint.sh | sh")
endif

.PHONY: fmt lint test install_deps clean
.PHONY: lint test install_deps clean

default: all

all: fmt test
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removing fmt here means that when people run make locally, they will no longer get errors for formatting errors. Considering the linter takes less tan 2 seconds to run, I suggest running it for make all.
If someone does not want to run it, they can do make test directly.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fmt is removed here because the Makefile target fmt is removed in this PR. We can add lint in this target, but that will require contributors to have golangci-lint installed before running make. I think it is better not to require having golangci-lint in order to make cobra. At the same time, it does not make sense to have an specific make target for one single linter but not for others. Overall, the users/developers interested in contributing code will always want to make lint test clean.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I started contributing I would do make and think everything was ok. It was only later that I realized this did not include the linters, so now I run make lint manually.

But I agree that making golangc-lint a requirement may be annoying. But it will also be annoying to only notice the bad formatting once CI is run...

I'm not sure what is best...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO modifying the sources and building cobra are not directly related to contributing code back to the upstream. That is, users might want to use and modify this open source tool regardless of the linting rules enforced in this repository. From this point of view, the requirement/need to use golangci-lint belongs to contributing guidelines: https://github.com/spf13/cobra/blob/main/CONTRIBUTING.md. Everyone should read the contributing guidelines before submitting a PR to a project.

all: test

fmt:
$(info ******************** checking formatting ********************)
@test -z $(shell gofmt -l $(SRC)) || (gofmt -d $(SRC); exit 1)
Copy link
Collaborator

Choose a reason for hiding this comment

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

So we no longer need gofmt because our linter handles it? We should change this to be a call out to the golangci-lint commandline utility then.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See the 'lint' target in the same Makefile.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah gotcha - but is golangci-lint going to format poorly formatted code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. It will check if the format is correct. That's what the current fmt Makefile target does:

cobra/Makefile

Lines 19 to 20 in a281c8b

$(info ******************** checking formatting ********************)
@test -z $(shell gofmt -l $(SRC)) || (gofmt -d $(SRC); exit 1)

This PR does not introduce any change in that regard.


lint:
$(info ******************** running lint tools ********************)
golangci-lint run -v
install_deps:
$(info ******************** downloading dependencies ********************)
go get -v ./...

test: install_deps
$(info ******************** running tests ********************)
Expand All @@ -27,9 +23,9 @@ richtest: install_deps
$(info ******************** running tests with kyoh86/richgo ********************)
richgo test -v ./...

install_deps:
$(info ******************** downloading dependencies ********************)
go get -v ./...
lint:
$(info ******************** running lint tools ********************)
golangci-lint run -v

clean:
rm -rf $(BIN)
2 changes: 1 addition & 1 deletion completions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1261,7 +1261,7 @@ func TestValidArgsFuncCmdContext(t *testing.T) {
}
rootCmd.AddCommand(childCmd)

//nolint:golint,staticcheck // We can safely use a basic type as key in tests.
//nolint:golint,revive,staticcheck // We can safely use a basic type as key in tests.
ctx := context.WithValue(context.Background(), "testKey", "123")

// Test completing an empty string on the childCmd
Expand Down
Loading