[linting] enable more rules for revive and address issues (#849)

hezhizhen · web-flow · commit 0f1c44714a9f · 2023-03-31T10:36:04.000-07:00
## Summary

1. Enable more rules for revive.
2. Address existing issues.

## How was it tested?

devbox run lint
diff --git a/.golangci.yml b/.golangci.yml
@@ -35,8 +35,25 @@ linters-settings:
   errorlint:
     errorf: false
   revive:
-    rules:
+    rules: # https://github.com/mgechev/revive/blob/master/RULES_DESCRIPTIONS.md
+      - name: atomic
+      - name: bare-return
+      - name: bool-literal-in-expr
+      - name: cognitive-complexity
+        arguments:
+          - 36 # TODO: gradually reduce it
+      - name: datarace
+      - name: duplicated-imports
+      - name: early-return
+      - name: error-return
+      - name: error-strings
+      - name: if-return
       - name: indent-error-flow
+      - name: range-val-address
+      - name: receiver-naming
+      - name: time-naming
+      - name: var-naming
+      - name: unreachable-code
   varnamelen:
     max-distance: 10
     ignore-decls:
diff --git a/internal/boxcli/gen-docs.go b/internal/boxcli/gen-docs.go
@@ -21,7 +21,6 @@ func genDocsCmd() *cobra.Command {
 		Hidden: true,
 		Args:   cobra.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-
 			wd, err := os.Getwd()
 			if err != nil {
 				return errors.WithStack(err)
diff --git a/internal/boxcli/generate.go b/internal/boxcli/generate.go
@@ -86,8 +86,9 @@ func direnvCmd() *cobra.Command {
 	command := &cobra.Command{
 		Use:   "direnv",
 		Short: "Generate a .envrc file that integrates direnv with this devbox project",
-		Long:  "Generate a .envrc file that integrates direnv with this devbox project. Requires direnv to be installed.",
-		Args:  cobra.MaximumNArgs(0),
+		Long: "Generate a .envrc file that integrates direnv with this devbox project. " +
+			"Requires direnv to be installed.",
+		Args: cobra.MaximumNArgs(0),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return runGenerateCmd(cmd, args, flags)
 		},
diff --git a/internal/boxcli/init.go b/internal/boxcli/init.go
@@ -13,8 +13,10 @@ func InitCmd() *cobra.Command {
 	command := &cobra.Command{
 		Use:   "init [<dir>]",
 		Short: "Initialize a directory as a devbox project",
-		Long:  "Initialize a directory as a devbox project. This will create an empty devbox.json in the current directory. You can then add packages using `devbox add`",
-		Args:  cobra.MaximumNArgs(1),
+		Long: "Initialize a directory as a devbox project. " +
+			"This will create an empty devbox.json in the current directory. " +
+			"You can then add packages using `devbox add`",
+		Args: cobra.MaximumNArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
 			return runInitCmd(cmd, args)
 		},
diff --git a/internal/boxcli/log.go b/internal/boxcli/log.go
@@ -7,7 +7,6 @@ import (
 )
 
 func LogCmd() *cobra.Command {
-
 	cmd := &cobra.Command{
 		Use:    "log <event-name> [<event-specific-args>]",
 		Hidden: true,
diff --git a/internal/boxcli/midcobra/telemetry.go b/internal/boxcli/midcobra/telemetry.go
@@ -103,7 +103,6 @@ type event struct {
 // newEventIfValid creates a new telemetry event, but returns nil if we cannot construct
 // a valid event.
 func (m *telemetryMiddleware) newEventIfValid(cmd *cobra.Command, args []string, runErr error) *event {
-
 	subcmd, flags, parseErr := getSubcommand(cmd, args)
 	if parseErr != nil {
 		// Ignore invalid commands
diff --git a/internal/boxcli/run.go b/internal/boxcli/run.go
@@ -80,15 +80,13 @@ func parseScriptArgs(args []string, flags runCmdFlags) (string, string, []string
 		return "", "", nil, err
 	}
 
-	script := ""
-	var scriptArgs []string
-	if len(args) >= 1 {
-		script = args[0]
-		scriptArgs = args[1:]
-	} else {
+	if len(args) == 0 {
 		// this should never happen because cobra should prevent it, but it's better to be defensive.
 		return "", "", nil, usererr.New("no command or script provided")
 	}
 
+	script := args[0]
+	scriptArgs := args[1:]
+
 	return path, script, scriptArgs, nil
 }
diff --git a/internal/cloud/cloud.go b/internal/cloud/cloud.go
@@ -74,7 +74,7 @@ func ensureVMForUser(vmHostname string, w io.Writer, username string, sshCmd *op
 
 			// We save the username to local file only after we get a successful response
 			// from the gateway, because the gateway will verify that the user's SSH keys
-			// match their claimed username from github.
+			// match their claimed username from GitHub.
 			err = openssh.SaveGithubUsernameToLocalFile(username)
 			if err != nil {
 				debug.Log("Failed to save username: %v", err)
@@ -502,13 +502,13 @@ func parseVMEnvVar() (username string, vmHostname string) {
 	// DEVBOX_VM = <hostname>
 	if len(parts) == 1 {
 		vmHostname = parts[0]
-		return
+		return username, vmHostname
 	}
 
 	// DEVBOX_VM = <username>@<hostname>
 	username = parts[0]
 	vmHostname = parts[1]
-	return
+	return username, vmHostname
 }
 
 // Proof of concept: look for a gitignore file in the current directory.
diff --git a/internal/cloud/mutagen/type_test.go b/internal/cloud/mutagen/type_test.go
@@ -7,7 +7,6 @@ import (
 )
 
 func TestSanitizeSessionName(t *testing.T) {
-
 	testCases := []struct {
 		input     string
 		sanitized string
diff --git a/internal/cloud/openssh/sshshim/mutagen.go b/internal/cloud/openssh/sshshim/mutagen.go
@@ -102,7 +102,6 @@ func checkActiveVM(vmAddr string) (bool, error) {
 // vmAddressIfAny will seek to find the devbox-vm hostname if it exists
 // in the sshArgs. If not, it returns an empty string.
 func vmAddressIfAny(sshArgs []string) string {
-
 	const devboxVMAddressSuffix = "devbox-vms.internal"
 	for _, sshArg := range sshArgs {
 		if strings.HasSuffix(sshArg, devboxVMAddressSuffix) {
diff --git a/internal/cloud/openssh/username.go b/internal/cloud/openssh/username.go
@@ -25,7 +25,6 @@ func GithubUsernameFromLocalFile() (string, error) {
 }
 
 func SaveGithubUsernameToLocalFile(username string) error {
-
 	filePath, err := usernameFilePath()
 	if err != nil {
 		return errors.WithStack(err)
diff --git a/internal/cloud/user_test.go b/internal/cloud/user_test.go
@@ -5,7 +5,6 @@ import (
 )
 
 func TestParseUsernameFromErrorMessage(t *testing.T) {
-
 	testCases := []struct {
 		name       string
 		errMessage string
diff --git a/internal/impl/config_test.go b/internal/impl/config_test.go
@@ -68,7 +68,6 @@ func TestFindProjectDirFromParentDirSearch(t *testing.T) {
 }
 
 func TestFindParentDirAtPath(t *testing.T) {
-
 	testCases := []struct {
 		name        string
 		allDirs     string
diff --git a/internal/impl/devbox.go b/internal/impl/devbox.go
@@ -303,94 +303,83 @@ func (d *Devbox) GenerateDevcontainer(force bool) error {
 
 	// check if devcontainer.json or Dockerfile exist
 	filesExist := plansdk.FileExists(devContainerJSONPath) || plansdk.FileExists(dockerfilePath)
-
-	if force || !filesExist {
-		// create directory
-		err := os.MkdirAll(devContainerPath, os.ModePerm)
-		if err != nil {
-			return errors.WithStack(err)
-		}
-		// generate dockerfile
-		err = generate.CreateDockerfile(tmplFS, devContainerPath)
-		if err != nil {
-			return errors.WithStack(err)
-		}
-		// generate devcontainer.json
-		err = generate.CreateDevcontainer(devContainerPath, d.mergedPackages())
-		if err != nil {
-			return errors.WithStack(err)
-		}
-	} else {
+	if !force && filesExist {
 		return usererr.New(
 			"Files devcontainer.json or Dockerfile are already present in .devcontainer/. " +
 				"Remove the files or use --force to overwrite them.",
 		)
 	}
-	return nil
+
+	// create directory
+	err := os.MkdirAll(devContainerPath, os.ModePerm)
+	if err != nil {
+		return errors.WithStack(err)
+	}
+	// generate dockerfile
+	err = generate.CreateDockerfile(tmplFS, devContainerPath)
+	if err != nil {
+		return errors.WithStack(err)
+	}
+	// generate devcontainer.json
+	return errors.WithStack(generate.CreateDevcontainer(devContainerPath, d.mergedPackages()))
 }
 
 // GenerateDockerfile generates a Dockerfile that replicates the devbox shell
 func (d *Devbox) GenerateDockerfile(force bool) error {
 	dockerfilePath := filepath.Join(d.projectDir, "Dockerfile")
 	// check if Dockerfile doesn't exist
 	filesExist := plansdk.FileExists(dockerfilePath)
-	if force || !filesExist {
-		// generate dockerfile
-		err := generate.CreateDockerfile(tmplFS, d.projectDir)
-		if err != nil {
-			return errors.WithStack(err)
-		}
-	} else {
+	if !force && filesExist {
 		return usererr.New(
 			"Dockerfile is already present in the current directory. " +
 				"Remove it or use --force to overwrite it.",
 		)
 	}
 
-	return nil
+	// generate dockerfile
+	return errors.WithStack(generate.CreateDockerfile(tmplFS, d.projectDir))
 }
 
 // GenerateEnvrc generates a .envrc file that makes direnv integration convenient
 func (d *Devbox) GenerateEnvrc(force bool, source string) error {
 	envrcfilePath := filepath.Join(d.projectDir, ".envrc")
 	filesExist := fileutil.Exists(envrcfilePath)
+	if !force && filesExist {
+		return usererr.New(
+			"A .envrc is already present in the current directory. " +
+				"Remove it or use --force to overwrite it.",
+		)
+	}
 	// confirm .envrc doesn't exist and don't overwrite an existing .envrc
-	if force || !filesExist {
-		if commandExists("direnv") {
-			// prompt for direnv allow
-			var result string
-			prompt := &survey.Input{
-				Message: "Do you want to enable direnv integration for this devbox project? [y/N]",
+	if commandExists("direnv") {
+		// prompt for direnv allow
+		var result string
+		prompt := &survey.Input{
+			Message: "Do you want to enable direnv integration for this devbox project? [y/N]",
+		}
+		err := survey.AskOne(prompt, &result)
+		if err != nil {
+			return errors.WithStack(err)
+		}
+
+		if strings.ToLower(result) == "y" {
+			// .envrc file creation
+			err := generate.CreateEnvrc(tmplFS, d.projectDir)
+			if err != nil {
+				return errors.WithStack(err)
 			}
-			err := survey.AskOne(prompt, &result)
+			cmd := exec.Command("direnv", "allow")
+			err = cmd.Run()
 			if err != nil {
 				return errors.WithStack(err)
 			}
-
-			if strings.ToLower(result) == "y" {
-				// .envrc file creation
-				err := generate.CreateEnvrc(tmplFS, d.projectDir)
-				if err != nil {
-					return errors.WithStack(err)
-				}
-				cmd := exec.Command("direnv", "allow")
-				err = cmd.Run()
-				if err != nil {
-					return errors.WithStack(err)
-				}
-			} else if source == "generate" {
-				// .envrc file creation
-				err := generate.CreateEnvrc(tmplFS, d.projectDir)
-				if err != nil {
-					return errors.WithStack(err)
-				}
+		} else if source == "generate" {
+			// .envrc file creation
+			err := generate.CreateEnvrc(tmplFS, d.projectDir)
+			if err != nil {
+				return errors.WithStack(err)
 			}
 		}
-	} else {
-		return usererr.New(
-			"A .envrc is already present in the current directory. " +
-				"Remove it or use --force to overwrite it.",
-		)
 	}
 
 	return nil
diff --git a/internal/impl/devbox_test.go b/internal/impl/devbox_test.go
@@ -28,7 +28,6 @@ func TestDevbox(t *testing.T) {
 	}
 }
 func testShellPlan(t *testing.T, testPath string) {
-
 	currentDir, err := os.Getwd()
 	require.New(t).NoError(err)
 
diff --git a/internal/impl/generate.go b/internal/impl/generate.go
@@ -157,7 +157,6 @@ var templateFuncs = template.FuncMap{
 }
 
 func makeFlakeFile(outPath string, plan *plansdk.ShellPlan) error {
-
 	flakeDir := filepath.Join(outPath, "flake")
 	err := writeFromTemplate(flakeDir, plan, "flake.nix")
 	if err != nil {
diff --git a/internal/impl/shell.go b/internal/impl/shell.go
@@ -130,7 +130,6 @@ func shellPath(nixpkgsCommitHash string) (path string, err error) {
 // initShellBinaryFields initializes the fields specific to the shell binary that will be used
 // for the devbox shell.
 func initShellBinaryFields(path string) *DevboxShell {
-
 	shell := &DevboxShell{binPath: path}
 	base := filepath.Base(path)
 	// Login shell
@@ -286,7 +285,6 @@ func (s *DevboxShell) shellRCOverrides(shellrc string) (extraEnv map[string]stri
 }
 
 func (s *DevboxShell) writeDevboxShellrc() (path string, err error) {
-
 	// We need a temp dir (as opposed to a temp file) because zsh uses
 	// ZDOTDIR to point to a new directory containing the .zshrc.
 	tmp, err := os.MkdirTemp("", "devbox")
@@ -357,7 +355,6 @@ func (s *DevboxShell) writeDevboxShellrc() (path string, err error) {
 //
 // We do not link the .{shell}rc files, since devbox modifies them. See writeDevboxShellrc
 func (s *DevboxShell) linkShellStartupFiles(shellSettingsDir string) {
-
 	// For now, we only need to do this for zsh shell
 	if s.name == shZsh {
 		// Useful explanation of zsh startup files: https://zsh.sourceforge.io/FAQ/zshfaq03.html#l20
diff --git a/internal/initrec/recommenders/java/java.go b/internal/initrec/recommenders/java/java.go
@@ -72,7 +72,7 @@ func (r *Recommender) packageManager() (string, error) {
 	} else if plansdk.FileExists(buildGradlePath) {
 		return GradleType, nil
 	} else {
-		return "", errors.New("Could not locate a Maven or Gradle file.")
+		return "", errors.New("could not locate a Maven or Gradle file")
 	}
 }
 
diff --git a/internal/initrec/recommenders/python/python_pip.go b/internal/initrec/recommenders/python/python_pip.go
@@ -20,7 +20,6 @@ func (r *RecommenderPip) IsRelevant() bool {
 	return plansdk.FileExists(filepath.Join(r.SrcDir, "requirements.txt"))
 }
 func (r *RecommenderPip) Packages() []string {
-
 	return []string{
 		"python3",
 	}
diff --git a/internal/initrec/recommenders/rust/rust.go b/internal/initrec/recommenders/rust/rust.go
@@ -34,7 +34,6 @@ func (r *Recommender) Packages() []string {
 // NOTE: `cargo build` succeeded with lowercase cargo.toml, but `cargo build --release`
 // will insist on `Cargo.toml`. We are lenient and tolerate both.
 func cargoTomlPath(srcDir string) string {
-
 	cargoTomlPath := filepath.Join(srcDir, cargoToml)
 	if plansdk.FileExists(cargoTomlPath) {
 		return cargoTomlPath
diff --git a/internal/initrec/recommenders/zig/zig.go b/internal/initrec/recommenders/zig/zig.go
@@ -1,5 +1,6 @@
 // Copyright 2023 Jetpack Technologies Inc and contributors. All rights reserved.
 // Use of this source code is governed by the license in the LICENSE file.
+
 package zig
 
 import (
@@ -15,7 +16,6 @@ type Recommender struct {
 var _ recommenders.Recommender = (*Recommender)(nil)
 
 func (r *Recommender) IsRelevant() bool {
-
 	a, err := plansdk.NewAnalyzer(r.SrcDir)
 	if err != nil {
 		// We should log that an error has occurred.
diff --git a/internal/nix/profile.go b/internal/nix/profile.go
diff --git a/internal/nix/profile_test.go b/internal/nix/profile_test.go
diff --git a/internal/nix/writer.go b/internal/nix/writer.go
diff --git a/internal/plugin/plugin.go b/internal/plugin/plugin.go
diff --git a/internal/services/services.go b/internal/services/services.go
diff --git a/internal/telemetry/segment.go b/internal/telemetry/segment.go
diff --git a/internal/wrapnix/wrapper.go b/internal/wrapnix/wrapper.go
diff --git a/testscripts/testrunner/examplesrunner.go b/testscripts/testrunner/examplesrunner.go

Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,6 @@ import (`
`7`	`7`	`)`
`8`	`8`
`9`	`9`	`func LogCmd() *cobra.Command {`
`10`		`-`
`11`	`10`	`cmd := &cobra.Command{`
`12`	`11`	`Use: "log <event-name> [<event-specific-args>]",`
`13`	`12`	`Hidden: true,`
Original file line number	Diff line number	Diff line change
`@@ -80,15 +80,13 @@ func parseScriptArgs(args []string, flags runCmdFlags) (string, string, []string`
`80`	`80`	`return "", "", nil, err`
`81`	`81`	`}`
`82`	`82`
`83`		`- script := ""`
`84`		`- var scriptArgs []string`
`85`		`- if len(args) >= 1 {`
`86`		`- script = args[0]`
`87`		`- scriptArgs = args[1:]`
`88`		`- } else {`
	`83`	`+ if len(args) == 0 {`
`89`	`84`	`// this should never happen because cobra should prevent it, but it's better to be defensive.`
`90`	`85`	`return "", "", nil, usererr.New("no command or script provided")`
`91`	`86`	`}`
`92`	`87`
	`88`	`+ script := args[0]`
	`89`	`+ scriptArgs := args[1:]`
	`90`	`+`
`93`	`91`	`return path, script, scriptArgs, nil`
`94`	`92`	`}`
Original file line number	Diff line number	Diff line change
`@@ -7,7 +7,6 @@ import (`
`7`	`7`	`)`
`8`	`8`
`9`	`9`	`func TestSanitizeSessionName(t *testing.T) {`
`10`		`-`
`11`	`10`	`testCases := []struct {`
`12`	`11`	`input string`
`13`	`12`	`sanitized string`