Merge pull request #483 from numtide/more-linter

Enable more golangci-linter

Author: brianmcgee

.golangci.yml

@@ -1,39 +1,23 @@
 # taken from https://github.com/nix-community/go-nix/blob/main/.golangci.yml
 linters:
-    enable:
-        - errname
-        - exhaustive
-        - gci
-        - gochecknoglobals
-        - gochecknoinits
-        - goconst
-        - godot
-        - gofumpt
-        - goheader
-        - goimports
-        - gosec
-        - importas
-        - ireturn
-        - lll
-        - makezero
-        - misspell
-        - nakedret
-        - nestif
-        - nilerr
-        - nilnil
-        - nlreturn
-        - noctx
-        - nolintlint
-        - prealloc
-        - predeclared
-        - revive
-        - rowserrcheck
-        - stylecheck
-        - tagliatelle
-        - tenv
-        - testpackage
-        - unconvert
-        - unparam
-        - wastedassign
-        - whitespace
-        - wsl
+    enable-all: true
+    disable:
+        - depguard
+        - execinquery
+        - exhaustruct
+        - exportloopref
+        - funlen
+        - godox
+        - gomnd
+        - mnd
+        - varnamelen
+        - forbidigo
+        - gocognit
+        - gocyclo
+        - cyclop
+        - err113
+        - maintidx
+        # would be nice to have but too many tests depend on environment variables, which is not allowed for t.Parallel()
+        - paralleltest
+        # would be also nice to enable because I also found some cases confusing
+        - nonamedreturns

cmd/format/format.go

@@ -123,7 +123,7 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 
 	if walkType == walk.Stdin && len(paths) != 1 {
 		// check we have only received one path arg which we use for the file extension / matching to formatters
-		return fmt.Errorf("exactly one path should be specified when using the --stdin flag")
+		return errors.New("exactly one path should be specified when using the --stdin flag")
 	}
 
 	// checks all paths are contained within the tree root and exist
@@ -136,7 +136,7 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 
 		relativePath, err := filepath.Rel(cfg.TreeRoot, absolutePath)
 		if err != nil {
-			return fmt.Errorf("error computing relative path from %s to %s: %s", cfg.TreeRoot, absolutePath, err)
+			return fmt.Errorf("error computing relative path from %s to %s: %w", cfg.TreeRoot, absolutePath, err)
 		}
 
 		if strings.HasPrefix(relativePath, "..") {
@@ -204,7 +204,7 @@ func Run(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, paths []string)
 
 	if formatErr != nil {
 		// return an error if any formatting failures were detected
-		return formatErr
+		return formatErr //nolint:wrapcheck
 	} else if cfg.FailOnChange && statz.Value(stats.Changed) != 0 {
 		// if fail on change has been enabled, check that no files were actually changed, throwing an error if so
 		return ErrFailOnChange

cmd/root.go

@@ -32,7 +32,7 @@ func NewRoot() (*cobra.Command, *stats.Stats) {
 
 	// create out root command
 	cmd := &cobra.Command{
-		Use:     fmt.Sprintf("%s <paths...>", build.Name),
+		Use:     build.Name + " <paths...>",
 		Short:   "One CLI to format your repo",
 		Version: build.Version,
 		RunE: func(cmd *cobra.Command, args []string) error {
@@ -91,7 +91,12 @@ func runE(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, args []string)
 	if init, err := flags.GetBool("init"); err != nil {
 		return fmt.Errorf("failed to read init flag: %w", err)
 	} else if init {
-		return _init.Run()
+		err := _init.Run()
+		if err != nil {
+			return fmt.Errorf("failed to run init command: %w", err)
+		}
+
+		return nil
 	}
 
 	// otherwise attempt to load the config file
@@ -149,5 +154,5 @@ func runE(v *viper.Viper, statz *stats.Stats, cmd *cobra.Command, args []string)
 	}
 
 	// format
-	return format.Run(v, statz, cmd, args)
+	return format.Run(v, statz, cmd, args) //nolint:wrapcheck
 }

cmd/root_test.go

@@ -80,7 +80,7 @@ func TestOnUnmatched(t *testing.T) {
 	// should exit with error when using fatal
 	t.Run("fatal", func(t *testing.T) {
 		errorFn := func(err error) {
-			as.ErrorContains(err, fmt.Sprintf("no formatter for path: %s", expectedPaths[0]))
+			as.ErrorContains(err, "no formatter for path: "+expectedPaths[0])
 		}
 
 		treefmt(t, withArgs("--on-unmatched", "fatal"), withError(errorFn))
@@ -718,6 +718,7 @@ func TestChangeWorkingDirectory(t *testing.T) {
 	})
 
 	execute := func(t *testing.T, configFile string, env bool) {
+		t.Helper()
 		t.Run(configFile, func(t *testing.T) {
 			// capture current cwd, so we can replace it after the test is finished
 			cwd, err := os.Getwd()
@@ -1075,7 +1076,7 @@ func TestCacheBusting(t *testing.T) {
 		formatter, err := os.OpenFile(scriptPath, os.O_WRONLY|os.O_APPEND, 0o755)
 		as.NoError(err, "failed to open elm formatter")
 
-		_, err = formatter.Write([]byte(" ")) // add some whitespace
+		_, err = formatter.WriteString(" ") // add some whitespace
 		as.NoError(err, "failed to append to elm formatter")
 		as.NoError(formatter.Close(), "failed to close elm formatter")
 
@@ -1839,6 +1840,8 @@ func withConfigFunc(path string, fn func() *config.Config) option {
 }
 
 func withStats(t *testing.T, expected map[stats.Type]int) option {
+	t.Helper()
+
 	return func(o *options) {
 		o.assertStats = func(s *stats.Stats) {
 			for k, v := range expected {
@@ -1855,6 +1858,8 @@ func withError(fn func(error)) option {
 }
 
 func withNoError(t *testing.T) option {
+	t.Helper()
+
 	return func(o *options) {
 		o.assertError = func(err error) {
 			require.NoError(t, err)

config/config.go

@@ -14,20 +14,20 @@ import (
 // Config is used to represent the list of configured Formatters.
 type Config struct {
 	AllowMissingFormatter bool     `mapstructure:"allow-missing-formatter" toml:"allow-missing-formatter,omitempty"`
-	CI                    bool     `mapstructure:"ci" toml:"-"`          // not allowed in config
-	ClearCache            bool     `mapstructure:"clear-cache" toml:"-"` // not allowed in config
-	CPUProfile            string   `mapstructure:"cpu-profile" toml:"cpu-profile,omitempty"`
-	Excludes              []string `mapstructure:"excludes" toml:"excludes,omitempty"`
-	FailOnChange          bool     `mapstructure:"fail-on-change" toml:"fail-on-change,omitempty"`
-	Formatters            []string `mapstructure:"formatters" toml:"formatters,omitempty"`
-	NoCache               bool     `mapstructure:"no-cache" toml:"-"` // not allowed in config
-	OnUnmatched           string   `mapstructure:"on-unmatched" toml:"on-unmatched,omitempty"`
-	TreeRoot              string   `mapstructure:"tree-root" toml:"tree-root,omitempty"`
-	TreeRootFile          string   `mapstructure:"tree-root-file" toml:"tree-root-file,omitempty"`
-	Verbose               uint8    `mapstructure:"verbose" toml:"verbose,omitempty"`
-	Walk                  string   `mapstructure:"walk" toml:"walk,omitempty"`
-	WorkingDirectory      string   `mapstructure:"working-dir" toml:"-"`
-	Stdin                 bool     `mapstructure:"stdin" toml:"-"` // not allowed in config
+	CI                    bool     `mapstructure:"ci"                      toml:"-"` // not allowed in config
+	ClearCache            bool     `mapstructure:"clear-cache"             toml:"-"` // not allowed in config
+	CPUProfile            string   `mapstructure:"cpu-profile"             toml:"cpu-profile,omitempty"`
+	Excludes              []string `mapstructure:"excludes"                toml:"excludes,omitempty"`
+	FailOnChange          bool     `mapstructure:"fail-on-change"          toml:"fail-on-change,omitempty"`
+	Formatters            []string `mapstructure:"formatters"              toml:"formatters,omitempty"`
+	NoCache               bool     `mapstructure:"no-cache"                toml:"-"` // not allowed in config
+	OnUnmatched           string   `mapstructure:"on-unmatched"            toml:"on-unmatched,omitempty"`
+	TreeRoot              string   `mapstructure:"tree-root"               toml:"tree-root,omitempty"`
+	TreeRootFile          string   `mapstructure:"tree-root-file"          toml:"tree-root-file,omitempty"`
+	Verbose               uint8    `mapstructure:"verbose"                 toml:"verbose,omitempty"`
+	Walk                  string   `mapstructure:"walk"                    toml:"walk,omitempty"`
+	WorkingDirectory      string   `mapstructure:"working-dir"             toml:"-"`
+	Stdin                 bool     `mapstructure:"stdin"                   toml:"-"` // not allowed in config
 
 	FormatterConfigs map[string]*Formatter `mapstructure:"formatter" toml:"formatter,omitempty"`
 

config/config_test.go

@@ -670,7 +670,7 @@ func TestSampleConfigFile(t *testing.T) {
 	as.True(ok, "shfmt formatter not found")
 	as.Equal("shfmt", shfmt.Command)
 	as.Equal(2, shfmt.Priority)
-	as.Equal(shfmt.Options, []string{"-i", "2", "-s", "-w"})
+	as.Equal([]string{"-i", "2", "-s", "-w"}, shfmt.Options)
 	as.Equal([]string{"*.sh"}, shfmt.Includes)
 	as.Nil(shfmt.Excludes)
 

format/formatter.go

@@ -8,6 +8,7 @@ import (
 	"os"
 	"os/exec"
 	"regexp"
+	"strconv"
 	"strings"
 	"time"
 
@@ -61,7 +62,7 @@ func (f *Formatter) Hash(h hash.Hash) error {
 	// if options change, the outcome of applying the formatter might be different
 	h.Write([]byte(strings.Join(f.config.Options, " ")))
 	// if priority changes, the outcome of applying a sequence of formatters might be different
-	h.Write([]byte(fmt.Sprintf("%d", f.config.Priority)))
+	h.Write([]byte(strconv.Itoa(f.config.Priority)))
 
 	// stat the formatter's executable
 	info, err := os.Lstat(f.executable)
@@ -163,7 +164,7 @@ func newFormatter(
 	if cfg.Priority > 0 {
 		f.log = log.WithPrefix(fmt.Sprintf("formatter | %s[%d]", name, cfg.Priority))
 	} else {
-		f.log = log.WithPrefix(fmt.Sprintf("formatter | %s", name))
+		f.log = log.WithPrefix("formatter | " + name)
 	}
 
 	// check there is at least one include

test/test.go

@@ -29,13 +29,15 @@ func WriteConfig(t *testing.T, path string, cfg *config.Config) {
 }
 
 func TempExamples(t *testing.T) string {
+	t.Helper()
 	tempDir := t.TempDir()
 	TempExamplesInDir(t, tempDir)
 
 	return tempDir
 }
 
 func TempExamplesInDir(t *testing.T, dir string) {
+	t.Helper()
 	require.NoError(t, cp.Copy("../test/examples", dir), "failed to copy test data to dir")
 
 	// we have second precision mod time tracking, so we wait a second before returning, so we don't trigger false

walk/cache/cache.go

@@ -33,14 +33,17 @@ func Open(root string) (*bolt.DB, error) {
 	// open db
 	db, err := bolt.Open(path, 0o600, &bolt.Options{Timeout: 1 * time.Second})
 	if err != nil {
-		return nil, err
+		return nil, fmt.Errorf("failed to open cache db: %w", err)
 	}
 
 	// ensure bucket exist
 	err = db.Update(func(tx *bolt.Tx) error {
 		_, err := tx.CreateBucketIfNotExists([]byte(bucketPaths))
+		if err != nil {
+			return fmt.Errorf("failed to create bucket: %w", err)
+		}
 
-		return err
+		return nil
 	})
 	if err != nil {
 		return nil, fmt.Errorf("failed to create bucket: %w", err)
@@ -65,7 +68,17 @@ func deleteAll(bucket *bolt.Bucket) error {
 }
 
 func Clear(db *bolt.DB) error {
-	return db.Update(func(tx *bolt.Tx) error {
-		return deleteAll(PathsBucket(tx))
+	err := db.Update(func(tx *bolt.Tx) error {
+		err := deleteAll(PathsBucket(tx))
+		if err != nil {
+			return fmt.Errorf("failed to clear cache: %w", err)
+		}
+
+		return nil
 	})
+	if err != nil {
+		return fmt.Errorf("failed to clear cache: %w", err)
+	}
+
+	return nil
 }

walk/cached.go

@@ -99,7 +99,7 @@ func (c *CachedReader) Read(ctx context.Context, files []*File) (n int, err erro
 		n, err = c.delegate.Read(ctx, files)
 		c.log.Debugf("read %d files from delegate", n)
 
-		for i := 0; i < n; i++ {
+		for i := range n {
 			file := files[i]
 
 			file.CachedFormatSignature = bucket.Get([]byte(file.RelPath))
@@ -115,15 +115,15 @@ func (c *CachedReader) Read(ctx context.Context, files []*File) (n int, err erro
 		}
 
 		if errors.Is(err, io.EOF) {
-			return err
+			return err //nolint:wrapcheck
 		} else if err != nil {
 			return fmt.Errorf("failed to read files from delegate: %w", err)
 		}
 
 		return nil
 	})
 
-	return n, err
+	return n, err //nolint:wrapcheck
 }
 
 // Close waits for any processing to complete.
@@ -132,7 +132,12 @@ func (c *CachedReader) Close() error {
 	close(c.updateCh)
 
 	// wait for any pending releases to be processed
-	return c.eg.Wait()
+	err := c.eg.Wait()
+	if err != nil {
+		return fmt.Errorf("failed to wait for processing to complete: %w", err)
+	}
+
+	return nil
 }
 
 // NewCachedReader creates a cache Reader instance, backed by a bolt DB and delegating reads to delegate.