From 47beaf56815892e8c5881917171917df1ac54ac7 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 6 Feb 2024 09:29:23 -0800 Subject: [PATCH 1/7] ci/lint: Use golangci-lint Instead of home-rolled linting workflows, use golangci-lint to manage linting. See also uber-go/zap#1323 --- .github/workflows/go.yml | 54 +++++++++++++++++------ .golangci.yml | 70 +++++++++++++++++++++++++++++ Makefile | 95 +++++++++++++++++++--------------------- tools/go.mod | 5 +-- tools/go.sum | 6 --- tools/tools.go | 2 - 6 files changed, 156 insertions(+), 76 deletions(-) create mode 100644 .golangci.yml diff --git a/.github/workflows/go.yml b/.github/workflows/go.yml index 87bbd7150..f1ddeb63b 100644 --- a/.github/workflows/go.yml +++ b/.github/workflows/go.yml @@ -14,14 +14,12 @@ jobs: build: runs-on: ${{ matrix.os }} + name: Build and test + strategy: matrix: os: ["ubuntu-latest", "windows-latest"] go: ["1.20.x", "1.21.x"] - include: - - go: 1.21.x - os: "ubuntu-latest" - latest: true steps: - name: Checkout code @@ -31,21 +29,51 @@ jobs: uses: actions/setup-go@v5 with: go-version: ${{ matrix.go }} - cache: true - name: Download Dependencies run: go mod download - - name: Lint - if: matrix.latest - run: make lint - - name: Test run: make cover - - name: Test documentation - run: make cover COVER_MODULES=./docs - if: matrix.latest - - name: Upload coverage to codecov.io uses: codecov/codecov-action@v3 + + doctest: + runs-on: ubuntu-latest + name: Test documentation + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Setup Go + uses: actions/setup-go@v5 + with: + go-version: 1.21.x + + - name: Test + run: make cover COVER_MODULES=./docs + + lint: + name: Lint + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Setup Go + uses: actions/setup-go@v5 + with: + go-version: 1.21.x + cache: false # managed by golangci-lint + + - uses: golangci/golangci-lint-action@v3 + name: Install golangci-lint + with: + version: latest + args: --version # make lint will run the linter + + - run: make lint + name: Lint diff --git a/.golangci.yml b/.golangci.yml new file mode 100644 index 000000000..e6f8be83b --- /dev/null +++ b/.golangci.yml @@ -0,0 +1,70 @@ +output: + # Make output more digestible with quickfix in vim/emacs/etc. + sort-results: true + print-issued-lines: false + +linters: + # We'll track the golangci-lint default linters manually + # instead of letting them change without our control. + disable-all: true + enable: + # golangci-lint defaults: + - gosimple + - govet + - ineffassign + - staticcheck + - unused + + # Our own extras: + - gofumpt + - nolintlint # lints nolint directives + - revive + - errorlint + +linters-settings: + govet: + # These govet checks are disabled by default, but they're useful. + enable: + - niliness + - reflectvaluecompare + - sortslice + - unusedwrite + +issues: + # Print all issues reported by all linters. + max-issues-per-linter: 0 + max-same-issues: 0 + + # Don't ignore some of the issues that golangci-lint considers okay. + # This includes documenting all exported entities. + exclude-use-default: false + + exclude-rules: + # Don't warn on unused parameters. + # Parameter names are useful; replacing them with '_' is undesirable. + - linters: [revive] + text: 'unused-parameter: parameter \S+ seems to be unused, consider removing or renaming it as _' + + # staticcheck already has smarter checks for empty blocks. + # revive's empty-block linter has false positives. + # For example, as of writing this, the following is not allowed. + # for foo() { } + - linters: [revive] + text: 'empty-block: this block is empty, you can remove it' + + # It's okay if internal packages and examples in docs/ + # don't have package comments. + - linters: [revive] + path: '.+/internal/.+|^internal/.+|^docs/.+' + text: 'should have a package comment' + + # It's okay for tests to use dot imports. + - linters: [revive] + path: '_test\.go$' + text: 'should not use dot imports' + + # Ignore logger.Sync() errcheck failures in example_test.go + # since those are intended to be uncomplicated examples. + - linters: [errcheck] + path: example_test.go + text: 'Error return value of `logger.Sync` is not checked' diff --git a/Makefile b/Makefile index f03be60ed..aef997620 100644 --- a/Makefile +++ b/Makefile @@ -1,27 +1,27 @@ -export GOBIN ?= $(shell pwd)/bin +# Directory containing the Makefile. +PROJECT_ROOT = $(dir $(abspath $(lastword $(MAKEFILE_LIST)))) + +export GOBIN ?= $(PROJECT_ROOT)/bin +export PATH := $(GOBIN):$(PATH) -GOLINT = $(GOBIN)/golint -STATICCHECK = $(GOBIN)/staticcheck FXLINT = $(GOBIN)/fxlint MDOX = $(GOBIN)/mdox -GO_FILES = $(shell \ - find . '(' -path '*/.*' -o -path './vendor' -o -path '*/testdata/*' ')' -prune \ - -o -name '*.go' -print | cut -b3-) - MODULES = . ./tools ./docs ./internal/e2e # 'make cover' should not run on docs by default. # We run that separately explicitly on a specific platform. COVER_MODULES ?= $(filter-out ./docs,$(MODULES)) +.PHONY: all +all: build lint test + .PHONY: build build: go build ./... -.PHONY: install -install: - go mod download +.PHONY: lint +lint: golangci-lint tidy-lint license-lint fx-lint docs-lint .PHONY: test test: @@ -35,11 +35,41 @@ cover: go test -race -coverprofile=cover.out -coverpkg=./... ./... && \ go tool cover -html=cover.out -o cover.html) &&) true -$(GOLINT): tools/go.mod - cd tools && go install golang.org/x/lint/golint +.PHONY: tidy +tidy: + @$(foreach dir,$(MODULES),(cd $(dir) && go mod tidy) &&) true + +.PHONY: docs +docs: + cd docs && yarn build -$(STATICCHECK): tools/go.mod - cd tools && go install honnef.co/go/tools/cmd/staticcheck +.PHONY: golangci-lint +golangci-lint: + @$(foreach mod,$(MODULES), \ + (cd $(mod) && \ + echo "[lint] golangci-lint: $(mod)" && \ + golangci-lint run --path-prefix $(mod)) &&) true + +.PHONY: tidy-lint +tidy-lint: + @$(foreach mod,$(MODULES), \ + (cd $(mod) && \ + echo "[lint] tidy: $(mod)" && \ + go mod tidy && \ + git diff --exit-code -- go.mod go.sum) &&) true + +.PHONY: license-lint +license-lint: + ./checklicense.sh + +.PHONY: fx-lint +fx-lint: $(FXLINT) + @$(FXLINT) ./... + +.PHONY: docs-lint +docs-lint: $(MDOX) + @echo "Checking documentation" + @make -C docs check $(MDOX): tools/go.mod cd tools && go install github.com/bwplotka/mdox @@ -47,40 +77,3 @@ $(MDOX): tools/go.mod $(FXLINT): tools/cmd/fxlint/main.go cd tools && go install go.uber.org/fx/tools/cmd/fxlint -.PHONY: lint -lint: $(GOLINT) $(STATICCHECK) $(FXLINT) docs-check - @rm -rf lint.log - @echo "Checking formatting..." - @gofmt -d -s $(GO_FILES) 2>&1 | tee lint.log - @echo "Checking vet..." - @$(foreach dir,$(MODULES),(cd $(dir) && go vet ./... 2>&1) &&) true | tee -a lint.log - @echo "Checking lint..." - @$(foreach dir,$(MODULES),(cd $(dir) && $(GOLINT) ./... 2>&1) &&) true | tee -a lint.log - @echo "Checking staticcheck..." - @$(foreach dir,$(MODULES),(cd $(dir) && $(STATICCHECK) ./... 2>&1) &&) true | tee -a lint.log - @echo "Checking fxlint..." - @$(FXLINT) ./... | tee -a lint.log - @echo "Checking for unresolved FIXMEs..." - @git grep -i fixme | grep -v -e vendor -e Makefile -e .md | tee -a lint.log - @echo "Checking for license headers..." - @./checklicense.sh | tee -a lint.log - @[ ! -s lint.log ] - @echo "Checking 'go mod tidy'..." - @make tidy - @if ! git diff --quiet; then \ - echo "'go mod tidy' resulted in changes or working tree is dirty:"; \ - git --no-pager diff; \ - fi - -.PHONY: docs -docs: - cd docs && yarn build - -.PHONY: docs-check -docs-check: $(MDOX) - @echo "Checking documentation" - @make -C docs check | tee -a lint.log - -.PHONY: tidy -tidy: - @$(foreach dir,$(MODULES),(cd $(dir) && go mod tidy) &&) true diff --git a/tools/go.mod b/tools/go.mod index 7b25e6f5e..044474fc0 100644 --- a/tools/go.mod +++ b/tools/go.mod @@ -4,13 +4,10 @@ go 1.20 require ( github.com/bwplotka/mdox v0.9.0 - golang.org/x/lint v0.0.0-20210508222113-6edffad5e616 golang.org/x/tools v0.17.0 - honnef.co/go/tools v0.4.6 ) require ( - github.com/BurntSushi/toml v1.2.1 // indirect github.com/Kunde21/markdownfmt/v2 v2.1.1-0.20221212171616-95b15aa5ffe4 // indirect github.com/PuerkitoBio/goquery v1.5.1 // indirect github.com/alecthomas/chroma v0.10.0 // indirect @@ -73,7 +70,7 @@ require ( github.com/theckman/yacspin v0.8.0 // indirect github.com/yuin/goldmark v1.4.13 // indirect github.com/yuin/goldmark-emoji v1.0.1 // indirect - golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a // indirect + golang.org/x/lint v0.0.0-20210508222113-6edffad5e616 // indirect golang.org/x/mod v0.14.0 // indirect golang.org/x/net v0.20.0 // indirect golang.org/x/sys v0.16.0 // indirect diff --git a/tools/go.sum b/tools/go.sum index 6796f5ffc..aecb86c03 100644 --- a/tools/go.sum +++ b/tools/go.sum @@ -57,8 +57,6 @@ github.com/Azure/go-autorest v11.1.2+incompatible/go.mod h1:r+4oMnoxhatjLLJ6zxSW github.com/Azure/go-autorest/tracing v0.1.0/go.mod h1:ROEEAFwXycQw7Sn3DXNtEedEvdeRAgDr0izn4z5Ij88= github.com/BurntSushi/locker v0.0.0-20171006230638-a6e239ea1c69/go.mod h1:L1AbZdiDllfyYH5l5OkAaZtk7VkWe89bPJFmnDBNHxg= github.com/BurntSushi/toml v0.3.1/go.mod h1:xHWCNGjB5oqiDr8zfno3MHue2Ht5sIBksp03qcyfWMU= -github.com/BurntSushi/toml v1.2.1 h1:9F2/+DoOYIOksmaJFPw1tGFy1eDnIJXg+UHjuD8lTak= -github.com/BurntSushi/toml v1.2.1/go.mod h1:CxXYINrC8qIiEnFrOxCa7Jy5BFHlXnUU2pbicEuybxQ= github.com/BurntSushi/xgb v0.0.0-20160522181843-27f122750802/go.mod h1:IVnqGOEym/WlBOVXweHU+Q+/VP0lqqI8lqeDx9IjBqo= github.com/GeertJohan/go.incremental v1.0.0/go.mod h1:6fAjUhbVuX1KcMD3c8TEgVUqmo4seqhv0i0kdATSkM0= github.com/GeertJohan/go.rice v1.0.0/go.mod h1:eH6gbSOAUv07dQuZVnBmoDP8mgsM1rtixis4Tib9if0= @@ -721,8 +719,6 @@ golang.org/x/exp v0.0.0-20191227195350-da58074b4299/go.mod h1:2RIsYlXP63K8oxa1u0 golang.org/x/exp v0.0.0-20200119233911-0405dc783f0a/go.mod h1:2RIsYlXP63K8oxa1u096TMicItID8zy7Y6sNkU49FU4= golang.org/x/exp v0.0.0-20200207192155-f17229e696bd/go.mod h1:J/WKrq2StrnmMY6+EHIKF9dgMWnmCNThgcyBT1FY9mM= golang.org/x/exp v0.0.0-20200224162631-6cc2880d07d6/go.mod h1:3jZMyOhIsHpP37uCMkUooju7aAi5cS1Q23tOzKc+0MU= -golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a h1:Jw5wfR+h9mnIYH+OtGT2im5wV1YGGDora5vTv/aa5bE= -golang.org/x/exp/typeparams v0.0.0-20221208152030-732eee02a75a/go.mod h1:AbB0pIl9nAr9wVwH+Z2ZpaocVmF5I4GyWCDIsVjR0bk= golang.org/x/image v0.0.0-20190227222117-0694c2d4d067/go.mod h1:kZ7UVZpmo3dzQBMxlp+ypCbDeSB+sBbTgSJuh5dn5js= golang.org/x/image v0.0.0-20190802002840-cff245a6509b/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= golang.org/x/image v0.0.0-20191214001246-9130b4cfad52/go.mod h1:FeLwcggjj3mMvU+oOTbSwawSJRM1uh48EjtB4UJZlP0= @@ -1111,8 +1107,6 @@ honnef.co/go/tools v0.0.0-20190523083050-ea95bdfd59fc/go.mod h1:rf3lG4BRIbNafJWh honnef.co/go/tools v0.0.1-2019.2.3/go.mod h1:a3bituU0lyd329TUQxRnasdCoJDkEUEAqEt0JzvZhAg= honnef.co/go/tools v0.0.1-2020.1.3/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k= honnef.co/go/tools v0.0.1-2020.1.4/go.mod h1:X/FiERA/W4tHapMX5mGpAtMSVEeEUOyHaw9vFzvIQ3k= -honnef.co/go/tools v0.4.6 h1:oFEHCKeID7to/3autwsWfnuv69j3NsfcXbvJKuIcep8= -honnef.co/go/tools v0.4.6/go.mod h1:+rnGS1THNh8zMwnd2oVOTL9QF6vmfyG6ZXBULae2uc0= pack.ag/amqp v0.8.0/go.mod h1:4/cbmt4EJXSKlG6LCfWHoqmN0uFdy5i/+YFz+fTfhV4= pack.ag/amqp v0.11.0/go.mod h1:4/cbmt4EJXSKlG6LCfWHoqmN0uFdy5i/+YFz+fTfhV4= rsc.io/binaryregexp v0.2.0/go.mod h1:qTv7/COck+e2FymRvadv62gMdZztPaShugOCi3I+8D8= diff --git a/tools/tools.go b/tools/tools.go index b88ecf497..b2561b7e3 100644 --- a/tools/tools.go +++ b/tools/tools.go @@ -26,6 +26,4 @@ package fx import ( // Tools we use during development. _ "github.com/bwplotka/mdox" - _ "golang.org/x/lint/golint" - _ "honnef.co/go/tools/cmd/staticcheck" ) From 94258f28448f8cec0a3186e096f66425d757a1e6 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 6 Feb 2024 09:30:09 -0800 Subject: [PATCH 2/7] fix(lint): gofumpt all files --- annotated.go | 2 -- annotated_test.go | 2 +- app_unixes.go | 6 ++++-- app_wasm.go | 6 ++++-- app_windows.go | 6 ++++-- decorate_test.go | 3 +-- fxevent/zap_test.go | 2 -- internal/lifecycle/lifecycle_test.go | 1 - invoke.go | 1 + module_test.go | 2 +- 10 files changed, 16 insertions(+), 15 deletions(-) diff --git a/annotated.go b/annotated.go index 301f77609..30a7f5fa2 100644 --- a/annotated.go +++ b/annotated.go @@ -178,7 +178,6 @@ func verifyValueQuote(value string) (string, error) { return "", errTagValueSyntaxEndingQuote } return value[i+1:], nil - } // Check whether the tag follows valid struct. @@ -211,7 +210,6 @@ func verifyAnnotateTag(tag string) error { tag = value } return nil - } // Given func(T1, T2, T3, ..., TN), this generates a type roughly diff --git a/annotated_test.go b/annotated_test.go index 30eb6d095..2ad36af65 100644 --- a/annotated_test.go +++ b/annotated_test.go @@ -1701,8 +1701,8 @@ func TestAnnotateApplySuccess(t *testing.T) { require.NoError(t, app.Err()) }) } - } + func assertApp( t *testing.T, app interface { diff --git a/app_unixes.go b/app_unixes.go index 5126e1618..8de1c509c 100644 --- a/app_unixes.go +++ b/app_unixes.go @@ -25,5 +25,7 @@ package fx import "golang.org/x/sys/unix" -const _sigINT = unix.SIGINT -const _sigTERM = unix.SIGTERM +const ( + _sigINT = unix.SIGINT + _sigTERM = unix.SIGTERM +) diff --git a/app_wasm.go b/app_wasm.go index 5e2ca8d01..fb4197a5a 100644 --- a/app_wasm.go +++ b/app_wasm.go @@ -25,5 +25,7 @@ package fx import "syscall" -const _sigINT = syscall.SIGINT -const _sigTERM = syscall.SIGTERM +const ( + _sigINT = syscall.SIGINT + _sigTERM = syscall.SIGTERM +) diff --git a/app_windows.go b/app_windows.go index b19a8d829..cbd571408 100644 --- a/app_windows.go +++ b/app_windows.go @@ -25,5 +25,7 @@ package fx import "golang.org/x/sys/windows" -const _sigINT = windows.SIGINT -const _sigTERM = windows.SIGTERM +const ( + _sigINT = windows.SIGINT + _sigTERM = windows.SIGTERM +) diff --git a/decorate_test.go b/decorate_test.go index 83c35506f..0518bf224 100644 --- a/decorate_test.go +++ b/decorate_test.go @@ -315,8 +315,7 @@ func TestDecorateSuccess(t *testing.T) { }) t.Run("decoration must execute when required by a member of group", func(t *testing.T) { - type Drinks interface { - } + type Drinks interface{} type Coffee struct { Type string Name string diff --git a/fxevent/zap_test.go b/fxevent/zap_test.go index 8e7987cba..a43ae170e 100644 --- a/fxevent/zap_test.go +++ b/fxevent/zap_test.go @@ -71,7 +71,6 @@ func TestZapLogger(t *testing.T) { }, }, { - name: "OnStopExecuted/Error", give: &OnStopExecuted{ FunctionName: "hook.onStart1", @@ -100,7 +99,6 @@ func TestZapLogger(t *testing.T) { }, }, { - name: "OnStartExecuted/Error", give: &OnStartExecuted{ FunctionName: "hook.onStart1", diff --git a/internal/lifecycle/lifecycle_test.go b/internal/lifecycle/lifecycle_test.go index 1b28191ff..2cde10658 100644 --- a/internal/lifecycle/lifecycle_test.go +++ b/internal/lifecycle/lifecycle_test.go @@ -329,7 +329,6 @@ func TestLifecycleStop(t *testing.T) { err = l.Stop(nil) require.Error(t, err) assert.Contains(t, err.Error(), "called OnStop with nil context") - }) } diff --git a/invoke.go b/invoke.go index 523aef83b..f60b7c9eb 100644 --- a/invoke.go +++ b/invoke.go @@ -89,6 +89,7 @@ func (o invokeOption) String() string { } return fmt.Sprintf("fx.Invoke(%s)", strings.Join(items, ", ")) } + func runInvoke(c container, i invoke) error { fn := i.Target switch fn := fn.(type) { diff --git a/module_test.go b/module_test.go index a1104b5fa..9b0fcc91a 100644 --- a/module_test.go +++ b/module_test.go @@ -414,7 +414,7 @@ func TestModuleSuccess(t *testing.T) { assert.Equal(t, []string{ "Supplied", "Provided", "Replaced", "Run", "Run", "LoggerInitialized", - //Invoke logged twice, once from child and another from grandchild + // Invoke logged twice, once from child and another from grandchild "Invoking", "Run", "Invoked", "Invoking", "Invoked", }, childSpy.EventTypes(), "events from grandchild also logged in child logger") From 83a70bfec6100c19275f4d4bccf9a1429ecbea04 Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 6 Feb 2024 09:31:33 -0800 Subject: [PATCH 3/7] fix(lint): var-declaration should omit type (revive) Fixes the following lint issues in code: ``` annotated.go:120:15: var-declaration: should omit type reflect.Type from declaration of var _typeOfError; it will be inferred from the right-hand side (revive) annotated.go:645:19: var-declaration: should omit type reflect.Type from declaration of var _typeOfLifecycle; it will be inferred from the right-hand side (revive) annotated.go:646:19: var-declaration: should omit type reflect.Type from declaration of var _typeOfContext; it will be inferred from the right-hand side (revive) internal/lifecycle/lifecycle.go:198:18: var-declaration: should omit type appState from declaration of var returnState; it will be inferred from the right-hand side (revive) ``` --- annotated.go | 8 ++++---- internal/lifecycle/lifecycle.go | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/annotated.go b/annotated.go index 30a7f5fa2..89f778783 100644 --- a/annotated.go +++ b/annotated.go @@ -117,8 +117,8 @@ type Annotation interface { } var ( - _typeOfError reflect.Type = reflect.TypeOf((*error)(nil)).Elem() - _nilError = reflect.Zero(_typeOfError) + _typeOfError = reflect.TypeOf((*error)(nil)).Elem() + _nilError = reflect.Zero(_typeOfError) ) // annotationError is a wrapper for an error that was encountered while @@ -642,8 +642,8 @@ func (la *lifecycleHookAnnotation) build(ann *annotated) (interface{}, error) { } var ( - _typeOfLifecycle reflect.Type = reflect.TypeOf((*Lifecycle)(nil)).Elem() - _typeOfContext reflect.Type = reflect.TypeOf((*context.Context)(nil)).Elem() + _typeOfLifecycle = reflect.TypeOf((*Lifecycle)(nil)).Elem() + _typeOfContext = reflect.TypeOf((*context.Context)(nil)).Elem() ) // buildHookInstaller returns a function that appends a hook to Lifecycle when called, diff --git a/internal/lifecycle/lifecycle.go b/internal/lifecycle/lifecycle.go index 9c0b9891d..5abca666d 100644 --- a/internal/lifecycle/lifecycle.go +++ b/internal/lifecycle/lifecycle.go @@ -195,7 +195,7 @@ func (l *Lifecycle) Start(ctx context.Context) error { l.startRecords = make(HookRecords, 0, len(l.hooks)) l.mu.Unlock() - var returnState appState = incompleteStart + returnState := incompleteStart defer func() { l.mu.Lock() l.state = returnState From 7bb08a9f6738a44cbaf147f9a21b66294f5d84bd Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 6 Feb 2024 09:33:59 -0800 Subject: [PATCH 4/7] fix(lint): Fix issues found by errorlint ``` app.go:550:14: type assertion on error will fail on wrapped errors. Use errors.As to check for specific errors (errorlint) decorate.go:219:81: non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint) ``` --- app.go | 7 +++++-- decorate.go | 2 +- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/app.go b/app.go index e07bd5bdc..728b07cb0 100644 --- a/app.go +++ b/app.go @@ -547,8 +547,11 @@ func (err errorWithGraph) Error() string { // VisualizeError returns the visualization of the error if available. func VisualizeError(err error) (string, error) { - if e, ok := err.(errWithGraph); ok && e.Graph() != "" { - return string(e.Graph()), nil + var erg errWithGraph + if errors.As(err, &erg) { + if g := erg.Graph(); g != "" { + return string(g), nil + } } return "", errors.New("unable to visualize error") } diff --git a/decorate.go b/decorate.go index 509134517..a056a7ec6 100644 --- a/decorate.go +++ b/decorate.go @@ -216,7 +216,7 @@ func runDecorator(c container, d decorator, opts ...dig.DecorateOption) (err err decorator := d.Target defer func() { if err != nil { - err = fmt.Errorf("fx.Decorate(%v) from:\n%+vFailed: %v", decorator, d.Stack, err) + err = fmt.Errorf("fx.Decorate(%v) from:\n%+vFailed: %w", decorator, d.Stack, err) } }() From 5df6ab17fece52e8ca00073b065cbcb8d4f349fe Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 6 Feb 2024 10:05:28 -0800 Subject: [PATCH 5/7] fix(lint): Public packages should have package comments Adds package comments missing from a couple public packages per revive. --- fxevent/doc.go | 73 ++++++++++++++++++++++++++++++++++++++++ fxtest/doc.go | 23 +++++++++++++ tools/cmd/fxlint/main.go | 5 +++ 3 files changed, 101 insertions(+) create mode 100644 fxevent/doc.go create mode 100644 fxtest/doc.go diff --git a/fxevent/doc.go b/fxevent/doc.go new file mode 100644 index 000000000..748268f7e --- /dev/null +++ b/fxevent/doc.go @@ -0,0 +1,73 @@ +// Copyright (c) 2024 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +// Package fxevent defines a means of changing how Fx logs internal events. +// +// # Changing the Logger +// +// By default, the [ConsoleLogger] is used, writing readable logs to stderr. +// +// You can use the fx.WithLogger option to change this behavior +// by providing a constructor that returns an alternative implementation +// of the [Logger] interface. +// +// fx.WithLogger(func(cfg *Config) Logger { +// return &MyFxLogger{...} +// }) +// +// Because WithLogger accepts a constructor, +// you can pull any other types and values from inside the container, +// allowing use of the same logger that your application uses. +// +// For example, if you're using Zap inside your application, +// you can use the [ZapLogger] implementation of the interface. +// +// fx.New( +// fx.Provide( +// zap.NewProduction, // provide a *zap.Logger +// ), +// fx.WithLogger( +// func(log *zap.Logger) fxevent.Logger { +// return &fxevent.ZapLogger{Logger: log} +// }, +// ), +// ) +// +// # Implementing a Custom Logger +// +// To implement a custom logger, you need to implement the [Logger] interface. +// The Logger.LogEvent method accepts an [Event] object. +// +// [Event] is a union type that represents all the different events that Fx can emit. +// You can use a type switch to handle each event type. +// See 'event.go' for a list of all the possible events. +// +// func (l *MyFxLogger) LogEvent(e fxevent.Event) { +// switch e := e.(type) { +// case *fxevent.OnStartExecuting: +// // ... +// // ... +// } +// } +// +// The events contain enough information for observability and debugging purposes. +// If you need more information in them, +// feel free to open an issue to discuss the addition. +package fxevent diff --git a/fxtest/doc.go b/fxtest/doc.go new file mode 100644 index 000000000..15d60fe05 --- /dev/null +++ b/fxtest/doc.go @@ -0,0 +1,23 @@ +// Copyright (c) 2024 Uber Technologies, Inc. +// +// Permission is hereby granted, free of charge, to any person obtaining a copy +// of this software and associated documentation files (the "Software"), to deal +// in the Software without restriction, including without limitation the rights +// to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +// copies of the Software, and to permit persons to whom the Software is +// furnished to do so, subject to the following conditions: +// +// The above copyright notice and this permission notice shall be included in +// all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +// IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +// FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +// AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +// LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN +// THE SOFTWARE. + +// Package fxtest provides utilities for testing Fx modules, +// and code that directly uses Fx. +package fxtest diff --git a/tools/cmd/fxlint/main.go b/tools/cmd/fxlint/main.go index 418f3f5e1..7d0bb2f66 100644 --- a/tools/cmd/fxlint/main.go +++ b/tools/cmd/fxlint/main.go @@ -18,6 +18,11 @@ // OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN // THE SOFTWARE. +// fxlint is a linter that verifies correct usage of Fx. +// +// Currently, the following passes are provided: +// +// - allfxevents: Verifies that all Fx events are handled by an fxevent.Logger. package main import ( From 7c2bd0390d5084d83b22339bb912b395edb46c4c Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 6 Feb 2024 10:07:17 -0800 Subject: [PATCH 6/7] fix: //lint:ignore => //nolint golangci-lint wants lint ignore directives in the form: //nolint:$name //nolint:$name // reason ``` internal/lifecycle/lifecycle_test.go:324:18: SA1012: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use (staticcheck) internal/lifecycle/lifecycle_test.go:329:16: SA1012: do not pass a nil Context, even if a function permits it; pass context.TODO if you are unsure about which Context to use (staticcheck) ``` --- internal/lifecycle/lifecycle_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/internal/lifecycle/lifecycle_test.go b/internal/lifecycle/lifecycle_test.go index 2cde10658..27519795b 100644 --- a/internal/lifecycle/lifecycle_test.go +++ b/internal/lifecycle/lifecycle_test.go @@ -320,13 +320,11 @@ func TestLifecycleStop(t *testing.T) { return nil }, }) - //lint:ignore SA1012 this test specifically tests for the lint failure - err := l.Start(nil) + err := l.Start(nil) //nolint:staticcheck // SA1012 this test specifically tests for the lint failure require.Error(t, err) assert.Contains(t, err.Error(), "called OnStart with nil context") - //lint:ignore SA1012 this test specifically tests for the lint failure - err = l.Stop(nil) + err = l.Stop(nil) //nolint:staticcheck // SA1012 this test specifically tests for the lint failure require.Error(t, err) assert.Contains(t, err.Error(), "called OnStop with nil context") }) From 4f1f46988169a7cda7f284b518ad4a5b2ad78e2f Mon Sep 17 00:00:00 2001 From: Abhinav Gupta Date: Tue, 6 Feb 2024 10:08:23 -0800 Subject: [PATCH 7/7] fix(lint): x += 1 => x++ ``` module_test.go:237:5: increment-decrement: should replace p.age += 1 with p.age++ (revive) module_test.go:242:6: increment-decrement: should replace p.age += 1 with p.age++ (revive) ``` --- module_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/module_test.go b/module_test.go index 9b0fcc91a..1a6fd9b71 100644 --- a/module_test.go +++ b/module_test.go @@ -234,12 +234,12 @@ func TestModuleSuccess(t *testing.T) { }), fx.Invoke(func(p *person) { assert.Equal(t, 2, p.age) - p.age += 1 + p.age++ }), fx.Module("module", fx.Invoke(func(p *person) { assert.Equal(t, 1, p.age) - p.age += 1 + p.age++ }), ), fx.Invoke(func(p *person) {