Description
Brief summary including motivation/context:
This is more of a meta-issue. I'm curious as to whether there's any interest in adding a linter to the project. I tested golangci-lint, and it runs in 1.685 seconds. I used the following config:
linters:
enable:
- golint
- gosec
- interfacer
- unconvert
- dupl
- goconst
- gocyclo
- goimports
- misspell
- scopelint
- gofmt
It showed the following output. Although most of these lint identifications aren't super important, some of them might catch places where the code could be simpler, or where the code might be ambiguous.
config/config_test.go:164:32: Using the variable on range scope `testCase` in function literal (scopelint)
actual, err := New([]string{testCase.env})
^
config/config_test.go:170:18: Using the variable on range scope `testCase` in function literal (scopelint)
if process != testCase.wantProcess {
^
config/config_test.go:171:42: Using the variable on range scope `testCase` in function literal (scopelint)
t.Errorf("Want process %v, got: %v", testCase.wantProcess, process)
^
executor/afterburn_runner.go:96:10: Error return value of `w.Write` is not checked (errcheck)
w.Write(bodyBytes)
^
executor/http_runner.go:94:21: Error return value of `cmd.Process.Signal` is not checked (errcheck)
cmd.Process.Signal(syscall.SIGTERM)
^
executor/http_runner.go:187:10: Error return value of `w.Write` is not checked (errcheck)
w.Write(bodyBytes)
^
executor/serializing_fork_runner.go:26:10: Error return value of `w.Write` is not checked (errcheck)
w.Write([]byte(err.Error()))
^
main.go:168:23: Error return value of `functionInvoker.Start` is not checked (errcheck)
functionInvoker.Start()
^
main.go:298:23: Error return value of `functionInvoker.Start` is not checked (errcheck)
functionInvoker.Start()
^
config/config.go:130:2: should use 'return <expr>' instead of 'if <expr> { return <bool> }; return <bool>' (gosimple)
if env[key] == "true" {
^
executor/http_runner.go:155:3: should use a simple channel send/receive instead of select with a single case (gosimple)
select {
^
main.go:31:26: printf-style function with dynamic format string and no further arguments should use print-style function instead (staticcheck)
fmt.Fprintf(os.Stderr, configErr.Error())
^
main.go:108:5: should omit comparison to bool constant, can be simplified to !suppressLock (gosimple)
if suppressLock == false {
^
main.go:129:3: redundant break statement (gosimple)
break
^
main.go:132:3: redundant break statement (gosimple)
break
^
main.go:135:3: redundant break statement (gosimple)
break
^
main.go:258:2: should merge variable declaration with assignment on next line (gosimple)
var envs []string
^
main.go:335:55: should omit comparison to bool constant, can be simplified to !lockFilePresent() (gosimple)
if atomic.LoadInt32(&acceptingConnections) == 0 || lockFilePresent() == false {
Any design changes
In the Dockerfile where we run the tests, we would have to install the linter, and run the linters.
Pros + Cons
Pros
- It makes it easier to avoid "dumb" code issues
- It simplifies the code
Cons
- It takes time to run
- Sometimes linters can be annoying
Effort required up front
It would require that we take the current repo, fix, or ignore, the linter issues, and then add a linter configuration, as well as configuration to invoke the linter at CI time.
Effort required for CI/CD, release, ongoing maintenance
Basically keeping up to date the linter, and the linter configuration.
Migration strategy / backwards-compatibility
See the effort required up front.
Activity