diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index df4287d4b..033801685 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -1766,9 +1766,9 @@ When `skipPackageNameChecks` is false (the default), you can configure to forbid using the values from the list as package names additionally to the standard meaningless ones: "common", "interfaces", "misc", "types", "util", "utils". -When `skipPackageNameCollisionWithGoStd` -(`skippackagenamecollisionwithgostd`, `skip-package-name-collision-with-go-std`) -is set to true, the rule disables checks on package names that collide +When `checkPackageNameCollisionWithGoStd` +(`checkpackagenamecollisionwithgostd`, `check-package-name-collision-with-go-std`) +is set to true, the rule checks for package names that collide with Go standard library packages. By default, the rule behaves exactly as the alternative in `golint` but optionally, you can relax it (see [golint/lint/issues/89](https://github.com/golang/lint/issues/89)). @@ -1817,7 +1817,7 @@ arguments = [[], [], [{ extra-bad-package-names = ["helpers", "models"] }]] ```toml [rule.var-naming] -arguments = [[], [], [{ skip-package-name-collision-with-go-std = true }]] +arguments = [[], [], [{ check-package-name-collision-with-go-std = true }]] ``` ## waitgroup-by-value diff --git a/logging/logger.go b/logging/logger.go index 2b4726de6..02c3f53fe 100644 --- a/logging/logger.go +++ b/logging/logger.go @@ -21,19 +21,30 @@ func GetLogger() (*slog.Logger, error) { return logger, nil } - debugModeEnabled := os.Getenv("DEBUG") != "" - if !debugModeEnabled { - // by default, suppress all logging output - return slog.New(slog.DiscardHandler), nil - } + // By default, suppress all logging output below level WARN, + // and only log to stderr. + leveler := new(slog.LevelVar) + leveler.Set(slog.LevelWarn) + opts := &slog.HandlerOptions{Level: leveler} - var err error - loggerFile, err = os.Create(logFile) - if err != nil { - return nil, err + var out io.Writer = os.Stderr + + debugModeEnabled := os.Getenv("DEBUG") != "" + if debugModeEnabled { + // In DEBUG mode, log all levels of output at level DEBUG and higher, + // to both stderr and the logFile. + leveler.Set(slog.LevelDebug) + + var err error + loggerFile, err = os.Create(logFile) + if err != nil { + return nil, err + } + + out = io.MultiWriter(os.Stderr, loggerFile) } - logger = slog.New(slog.NewTextHandler(io.MultiWriter(os.Stderr, loggerFile), nil)) + logger = slog.New(slog.NewTextHandler(out, opts)) logger.Info("Logger initialized", "logFile", logFile) @@ -42,6 +53,7 @@ func GetLogger() (*slog.Logger, error) { // Close closes the logger file if it was opened. func Close() error { + logger = nil if loggerFile == nil { return nil } diff --git a/logging/logger_test.go b/logging/logger_test.go index 63e3e82ae..b907d4aed 100644 --- a/logging/logger_test.go +++ b/logging/logger_test.go @@ -10,6 +10,11 @@ import ( func TestGetLogger(t *testing.T) { t.Run("no debug", func(t *testing.T) { t.Setenv("DEBUG", "") + t.Cleanup(func() { + if err := logging.Close(); err != nil { + t.Error(err) + } + }) logger, err := logging.GetLogger() if err != nil { diff --git a/rule/var_naming.go b/rule/var_naming.go index 4ab438fc1..a2db48d52 100644 --- a/rule/var_naming.go +++ b/rule/var_naming.go @@ -13,6 +13,7 @@ import ( "github.com/mgechev/revive/internal/astutils" "github.com/mgechev/revive/internal/rule" "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/logging" ) var knownNameExceptions = map[string]bool{ @@ -50,9 +51,9 @@ type VarNamingRule struct { extraBadPackageNames map[string]struct{} // inactive if skipPackageNameChecks is false pkgNameAlreadyChecked syncSet // set of packages names already checked - skipPackageNameCollisionWithGoStd bool // if true - disable checks for collisions with Go standard library package names + checkPackageNameCollisionWithGoStd bool // if true - checks for collisions with Go standard library package names // stdPackageNames holds the names of standard library packages excluding internal and vendor. - // populated only if skipPackageNameCollisionWithGoStd is false. + // populated only if checkPackageNameCollisionWithGoStd is true. // E.g., `net/http` stored as `http`, `math/rand/v2` - `rand` etc. stdPackageNames map[string]struct{} } @@ -117,11 +118,25 @@ func (r *VarNamingRule) Configure(arguments lint.Arguments) error { r.extraBadPackageNames[strings.ToLower(n)] = struct{}{} } case isRuleOption(k, "skipPackageNameCollisionWithGoStd"): - r.skipPackageNameCollisionWithGoStd = fmt.Sprint(v) == "true" + r.checkPackageNameCollisionWithGoStd = fmt.Sprint(v) != "true" + + slogger, err := logging.GetLogger() + if err == nil { + slogger.Warn( + "an option in the configuration is deprecated, attempting to interpret it and continue", + "deprecated_option", k, + "suggestion", "instead, use its inverse, checkPackageNameCollisionWithGoStd", + "interpreted_as", map[string]any{ + "checkPackageNameCollisionWithGoStd": r.checkPackageNameCollisionWithGoStd, + }, + ) + } + case isRuleOption(k, "checkPackageNameCollisionWithGoStd"): + r.checkPackageNameCollisionWithGoStd = fmt.Sprint(v) == "true" } } } - if !r.skipPackageNameCollisionWithGoStd && r.stdPackageNames == nil { + if r.checkPackageNameCollisionWithGoStd && r.stdPackageNames == nil { pkgs, err := gopackages.Load(nil, "std") if err != nil { return fmt.Errorf("load std packages: %w", err) @@ -215,7 +230,7 @@ func (r *VarNamingRule) applyPackageCheckRules(file *lint.File, onFailure func(f return } - if !r.skipPackageNameCollisionWithGoStd { + if r.checkPackageNameCollisionWithGoStd { if _, ok := r.stdPackageNames[pkgNameLower]; ok { onFailure(r.pkgNameFailure(pkgNameNode, "avoid package names that conflict with Go standard library package names")) } diff --git a/rule/var_naming_test.go b/rule/var_naming_test.go index d2decddb4..5c9b943cd 100644 --- a/rule/var_naming_test.go +++ b/rule/var_naming_test.go @@ -10,16 +10,16 @@ import ( func TestVarNamingRule_Configure(t *testing.T) { tests := []struct { - name string - arguments lint.Arguments - wantErr error - wantAllowList []string - wantBlockList []string - wantSkipInitialismNameChecks bool - wantAllowUpperCaseConst bool - wantSkipPackageNameChecks bool - wantBadPackageNames map[string]struct{} - wantSkipPackageNameCollisionWithGoStd bool + name string + arguments lint.Arguments + wantErr error + wantAllowList []string + wantBlockList []string + wantSkipInitialismNameChecks bool + wantAllowUpperCaseConst bool + wantSkipPackageNameChecks bool + wantBadPackageNames map[string]struct{} + wantCheckPackageNameCollisionWithGoStd bool }{ { name: "no arguments", @@ -37,21 +37,21 @@ func TestVarNamingRule_Configure(t *testing.T) { []any{"ID"}, []any{"VM"}, []any{map[string]any{ - "skipInitialismNameChecks": true, - "upperCaseConst": true, - "skipPackageNameChecks": true, - "extraBadPackageNames": []any{"helpers", "models"}, - "skipPackageNameCollisionWithGoStd": true, + "skipInitialismNameChecks": true, + "upperCaseConst": true, + "skipPackageNameChecks": true, + "extraBadPackageNames": []any{"helpers", "models"}, + "checkPackageNameCollisionWithGoStd": true, }}, }, - wantErr: nil, - wantAllowList: []string{"ID"}, - wantBlockList: []string{"VM"}, - wantSkipInitialismNameChecks: true, - wantAllowUpperCaseConst: true, - wantSkipPackageNameChecks: true, - wantBadPackageNames: map[string]struct{}{"helpers": {}, "models": {}}, - wantSkipPackageNameCollisionWithGoStd: true, + wantErr: nil, + wantAllowList: []string{"ID"}, + wantBlockList: []string{"VM"}, + wantSkipInitialismNameChecks: true, + wantAllowUpperCaseConst: true, + wantSkipPackageNameChecks: true, + wantBadPackageNames: map[string]struct{}{"helpers": {}, "models": {}}, + wantCheckPackageNameCollisionWithGoStd: true, }, { name: "valid lowercased arguments", @@ -59,21 +59,21 @@ func TestVarNamingRule_Configure(t *testing.T) { []any{"ID"}, []any{"VM"}, []any{map[string]any{ - "skipinitialismnamechecks": true, - "uppercaseconst": true, - "skippackagenamechecks": true, - "extrabadpackagenames": []any{"helpers", "models"}, - "skippackagenamecollisionwithgostd": true, + "skipinitialismnamechecks": true, + "uppercaseconst": true, + "skippackagenamechecks": true, + "extrabadpackagenames": []any{"helpers", "models"}, + "checkpackagenamecollisionwithgostd": true, }}, }, - wantErr: nil, - wantAllowList: []string{"ID"}, - wantBlockList: []string{"VM"}, - wantSkipInitialismNameChecks: true, - wantAllowUpperCaseConst: true, - wantSkipPackageNameChecks: true, - wantBadPackageNames: map[string]struct{}{"helpers": {}, "models": {}}, - wantSkipPackageNameCollisionWithGoStd: true, + wantErr: nil, + wantAllowList: []string{"ID"}, + wantBlockList: []string{"VM"}, + wantSkipInitialismNameChecks: true, + wantAllowUpperCaseConst: true, + wantSkipPackageNameChecks: true, + wantBadPackageNames: map[string]struct{}{"helpers": {}, "models": {}}, + wantCheckPackageNameCollisionWithGoStd: true, }, { name: "valid kebab-cased arguments", @@ -81,21 +81,21 @@ func TestVarNamingRule_Configure(t *testing.T) { []any{"ID"}, []any{"VM"}, []any{map[string]any{ - "skip-initialism-name-checks": true, - "upper-case-const": true, - "skip-package-name-checks": true, - "extra-bad-package-names": []any{"helpers", "models"}, - "skip-package-name-collision-with-go-std": true, + "skip-initialism-name-checks": true, + "upper-case-const": true, + "skip-package-name-checks": true, + "extra-bad-package-names": []any{"helpers", "models"}, + "check-package-name-collision-with-go-std": true, }}, }, - wantErr: nil, - wantAllowList: []string{"ID"}, - wantBlockList: []string{"VM"}, - wantSkipInitialismNameChecks: true, - wantAllowUpperCaseConst: true, - wantSkipPackageNameChecks: true, - wantBadPackageNames: map[string]struct{}{"helpers": {}, "models": {}}, - wantSkipPackageNameCollisionWithGoStd: true, + wantErr: nil, + wantAllowList: []string{"ID"}, + wantBlockList: []string{"VM"}, + wantSkipInitialismNameChecks: true, + wantAllowUpperCaseConst: true, + wantSkipPackageNameChecks: true, + wantBadPackageNames: map[string]struct{}{"helpers": {}, "models": {}}, + wantCheckPackageNameCollisionWithGoStd: true, }, { name: "invalid allowlist type", @@ -170,15 +170,23 @@ func TestVarNamingRule_Configure(t *testing.T) { if !reflect.DeepEqual(rule.extraBadPackageNames, tt.wantBadPackageNames) { t.Errorf("unexpected extraBadPackageNames: got = %v, want %v", rule.extraBadPackageNames, tt.wantBadPackageNames) } - if rule.skipPackageNameCollisionWithGoStd != tt.wantSkipPackageNameCollisionWithGoStd { - t.Errorf("unexpected skipPackageNameCollisionWithGoStd: got = %v, want %v", rule.skipPackageNameCollisionWithGoStd, tt.wantSkipPackageNameCollisionWithGoStd) + if rule.checkPackageNameCollisionWithGoStd != tt.wantCheckPackageNameCollisionWithGoStd { + t.Errorf("unexpected checkPackageNameCollisionWithGoStd: got = %v, want %v", rule.checkPackageNameCollisionWithGoStd, tt.wantCheckPackageNameCollisionWithGoStd) } }) } } func TestVarNamingRule_Configure_LoadStdPackages(t *testing.T) { - t.Run("loads std packages", func(t *testing.T) { + checkGoStdConfig := lint.Arguments{ + []any(nil), + []any(nil), + []any{map[string]any{ + "checkPackageNameCollisionWithGoStd": true, + }}, + } + + t.Run("nil config defaults to golint behavior and skips std packages", func(t *testing.T) { var rule VarNamingRule err := rule.Configure(nil) @@ -186,6 +194,27 @@ func TestVarNamingRule_Configure_LoadStdPackages(t *testing.T) { t.Fatalf("unexpected error: got = %v, want = nil", err) } + for _, pkg := range []string{"fmt", "http", "json", "rand", "io"} { + if _, ok := rule.stdPackageNames[pkg]; ok { + t.Errorf("expected package %q to be not loaded, but got loaded", pkg) + } + } + + for _, pkg := range []string{"v2", "internal", "vendor", "std", "text", "go"} { + if _, ok := rule.stdPackageNames[pkg]; ok { + t.Errorf("expected package %q to be not loaded, but got loaded", pkg) + } + } + }) + + t.Run("loads std packages", func(t *testing.T) { + var rule VarNamingRule + + err := rule.Configure(checkGoStdConfig) + if err != nil { + t.Fatalf("unexpected error: got = %v, want = nil", err) + } + for _, pkg := range []string{"fmt", "http", "json", "rand", "io"} { if _, ok := rule.stdPackageNames[pkg]; !ok { t.Errorf("expected package %q to be loaded, but got empty", pkg) @@ -202,12 +231,12 @@ func TestVarNamingRule_Configure_LoadStdPackages(t *testing.T) { t.Run("does not reload std packages on subsequent Configure calls", func(t *testing.T) { var rule VarNamingRule - err := rule.Configure(nil) + err := rule.Configure(checkGoStdConfig) if err != nil { t.Fatalf("unexpected error: got = %v, want = nil", err) } firstLoadLen := len(rule.stdPackageNames) - err = rule.Configure(nil) + err = rule.Configure(checkGoStdConfig) if err != nil { t.Fatalf("unexpected error: got = %v, want = nil", err) } @@ -223,7 +252,7 @@ func TestVarNamingRule_Configure_LoadStdPackages(t *testing.T) { []any{}, []any{}, []any{map[string]any{ - "skip-package-name-collision-with-go-std": true, + "check-package-name-collision-with-go-std": false, }}, }) if err != nil { diff --git a/test/var_naming_test.go b/test/var_naming_test.go index ee9247d25..2a6170e11 100644 --- a/test/var_naming_test.go +++ b/test/var_naming_test.go @@ -66,12 +66,18 @@ func TestVarNaming(t *testing.T) { }, }) testRule(t, "var_naming_top_level_pkg", &rule.VarNamingRule{}, &lint.RuleConfig{}) - testRule(t, "var_naming_std_lib_conflict", &rule.VarNamingRule{}, &lint.RuleConfig{}) + testRule(t, "var_naming_std_lib_conflict", &rule.VarNamingRule{}, &lint.RuleConfig{ + Arguments: []any{ + []any{}, + []any{}, + []any{map[string]any{"check-package-name-collision-with-go-std": true}}, + }, + }) testRule(t, "var_naming_std_lib_conflict_skip", &rule.VarNamingRule{}, &lint.RuleConfig{ Arguments: []any{ []any{}, []any{}, - []any{map[string]any{"skip-package-name-collision-with-go-std": true}}, + []any{map[string]any{"check-package-name-collision-with-go-std": false}}, }, }) }