diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 6868890..d3145f3 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -1118,12 +1118,14 @@ _Configuration_: N/A _Description_: This rule warns when [initialism](https://go.dev/wiki/CodeReviewComments#initialisms), [variable](https://go.dev/wiki/CodeReviewComments#variable-names) or [package](https://go.dev/wiki/CodeReviewComments#package-names) naming conventions are not followed. It ignores functions starting with `Example`, `Test`, `Benchmark`, and `Fuzz` in test files, preserving `golint` original behavior. -_Configuration_: This rule accepts two slices of strings and one optional slice with single map with named parameters. -(it's due to TOML hasn't "slice of any" and we keep backward compatibility with previous config version) -First slice is an allowlist and second one is a blocklist of initialisms. -In map, you can add boolean `upperCaseConst` (`uppercaseconst`, `upper-case-const`) parameter to allow `UPPER_CASE` for `const` -You can also add boolean `skipPackageNameChecks` (`skippackagenamechecks`, `skip-package-name-checks`) to skip package name checks. -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)) +_Configuration_: This rule accepts two slices of strings and one optional slice containing a single map with named parameters. +(This is because TOML does not support "slice of any," and we maintain backward compatibility with the previous configuration version). +The first slice is an allowlist, and the second one is a blocklist of initialisms. +In the map, you can add a boolean `upperCaseConst` (`uppercaseconst`, `upper-case-const`) parameter to allow `UPPER_CASE` for `const`. +You can also add a boolean `skipPackageNameChecks` (`skippackagenamechecks`, `skip-package-name-checks`) to skip package name checks. +When `skipPackageNameChecks` is false (the default), you can configure `extraBadPackageNames` (`extrabadpackagenames`, `extra-bad-package-names`) to forbid using the values from the list as package names additionally to the standard meaningless ones: "common", "interfaces", "misc", "types", "util", "utils". + +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)). Examples: @@ -1137,6 +1139,11 @@ Examples: arguments = [[], [], [{skipPackageNameChecks=true}]] ``` +```toml +[rule.var-naming] + arguments = [[], [], [{extraBadPackageNames=["helpers", "models"]}]] +``` + ```toml [rule.var-naming] arguments = [["ID"], ["VM"], [{upper-case-const=true}]] @@ -1147,6 +1154,11 @@ Examples: arguments = [[], [], [{skip-package-name-checks=true}]] ``` +```toml +[rule.var-naming] + arguments = [[], [], [{extra-bad-package-names=["helpers", "models"]}]] +``` + ## waitgroup-by-value _Description_: Function parameters that are passed by value, are in fact a copy of the original argument. Passing a copy of a `sync.WaitGroup` is usually not what the developer wants to do. diff --git a/rule/var_naming.go b/rule/var_naming.go index 2248d35..503ef5b 100644 --- a/rule/var_naming.go +++ b/rule/var_naming.go @@ -22,12 +22,26 @@ var knownNameExceptions = map[string]bool{ "kWh": true, } +// defaultBadPackageNames is the list of "bad" package names from https://go.dev/wiki/CodeReviewComments#package-names +// and https://go.dev/blog/package-names#bad-package-names. +// The rule warns about the usage of any package name in this list if skipPackageNameChecks is false. +// Values in the list should be lowercased. +var defaultBadPackageNames = map[string]struct{}{ + "common": {}, + "interfaces": {}, + "misc": {}, + "types": {}, + "util": {}, + "utils": {}, +} + // VarNamingRule lints the name of a variable. type VarNamingRule struct { allowList []string blockList []string - allowUpperCaseConst bool // if true - allows to use UPPER_SOME_NAMES for constants - skipPackageNameChecks bool + allowUpperCaseConst bool // if true - allows to use UPPER_SOME_NAMES for constants + skipPackageNameChecks bool // check for meaningless and user-defined bad package names + extraBadPackageNames map[string]struct{} // inactive if skipPackageNameChecks is false } // Configure validates the rule configuration, and configures the rule accordingly. @@ -70,27 +84,62 @@ func (r *VarNamingRule) Configure(arguments lint.Arguments) error { r.allowUpperCaseConst = fmt.Sprint(v) == "true" case isRuleOption(k, "skipPackageNameChecks"): r.skipPackageNameChecks = fmt.Sprint(v) == "true" + case isRuleOption(k, "extraBadPackageNames"): + extraBadPackageNames, ok := v.([]string) + if !ok { + return fmt.Errorf("invalid third argument to the var-naming rule. Expecting extraBadPackageNames of type slice of strings, but %T", v) + } + for _, name := range extraBadPackageNames { + if r.extraBadPackageNames == nil { + r.extraBadPackageNames = map[string]struct{}{} + } + r.extraBadPackageNames[strings.ToLower(name)] = struct{}{} + } } } } return nil } -func (*VarNamingRule) applyPackageCheckRules(walker *lintNames) { +func (r *VarNamingRule) applyPackageCheckRules(walker *lintNames) { + node := walker.fileAst.Name + packageName := node.Name + lowerPackageName := strings.ToLower(packageName) + + if _, ok := r.extraBadPackageNames[lowerPackageName]; ok { + walker.onFailure(lint.Failure{ + Failure: "avoid bad package names", + Confidence: 1, + Node: node, + Category: lint.FailureCategoryNaming, + }) + return + } + + if _, ok := defaultBadPackageNames[lowerPackageName]; ok { + walker.onFailure(lint.Failure{ + Failure: "avoid meaningless package names", + Confidence: 1, + Node: node, + Category: lint.FailureCategoryNaming, + }) + return + } + // Package names need slightly different handling than other names. - if strings.Contains(walker.fileAst.Name.Name, "_") && !strings.HasSuffix(walker.fileAst.Name.Name, "_test") { + if strings.Contains(packageName, "_") && !strings.HasSuffix(packageName, "_test") { walker.onFailure(lint.Failure{ Failure: "don't use an underscore in package name", Confidence: 1, - Node: walker.fileAst.Name, + Node: node, Category: lint.FailureCategoryNaming, }) } - if anyCapsRE.MatchString(walker.fileAst.Name.Name) { + if anyCapsRE.MatchString(packageName) { walker.onFailure(lint.Failure{ - Failure: fmt.Sprintf("don't use MixedCaps in package name; %s should be %s", walker.fileAst.Name.Name, strings.ToLower(walker.fileAst.Name.Name)), + Failure: fmt.Sprintf("don't use MixedCaps in package name; %s should be %s", packageName, lowerPackageName), Confidence: 1, - Node: walker.fileAst.Name, + Node: node, Category: lint.FailureCategoryNaming, }) } diff --git a/rule/var_naming_test.go b/rule/var_naming_test.go index 122c453..7d901f2 100644 --- a/rule/var_naming_test.go +++ b/rule/var_naming_test.go @@ -2,6 +2,7 @@ package rule import ( "errors" + "reflect" "testing" "github.com/mgechev/revive/lint" @@ -16,6 +17,7 @@ func TestVarNamingRule_Configure(t *testing.T) { wantBlockList []string wantAllowUpperCaseConst bool wantSkipPackageNameChecks bool + wantBadPackageNames map[string]struct{} }{ { name: "no arguments", @@ -34,6 +36,7 @@ func TestVarNamingRule_Configure(t *testing.T) { []any{map[string]any{ "upperCaseConst": true, "skipPackageNameChecks": true, + "extraBadPackageNames": []string{"helpers", "models"}, }}, }, wantErr: nil, @@ -41,6 +44,7 @@ func TestVarNamingRule_Configure(t *testing.T) { wantBlockList: []string{"VM"}, wantAllowUpperCaseConst: true, wantSkipPackageNameChecks: true, + wantBadPackageNames: map[string]struct{}{"helpers": {}, "models": {}}, }, { name: "valid lowercased arguments", @@ -50,6 +54,7 @@ func TestVarNamingRule_Configure(t *testing.T) { []any{map[string]any{ "uppercaseconst": true, "skippackagenamechecks": true, + "extrabadpackagenames": []string{"helpers", "models"}, }}, }, wantErr: nil, @@ -57,6 +62,7 @@ func TestVarNamingRule_Configure(t *testing.T) { wantBlockList: []string{"VM"}, wantAllowUpperCaseConst: true, wantSkipPackageNameChecks: true, + wantBadPackageNames: map[string]struct{}{"helpers": {}, "models": {}}, }, { name: "valid kebab-cased arguments", @@ -66,6 +72,7 @@ func TestVarNamingRule_Configure(t *testing.T) { []any{map[string]any{ "upper-case-const": true, "skip-package-name-checks": true, + "extra-bad-package-names": []string{"helpers", "models"}, }}, }, wantErr: nil, @@ -73,6 +80,7 @@ func TestVarNamingRule_Configure(t *testing.T) { wantBlockList: []string{"VM"}, wantAllowUpperCaseConst: true, wantSkipPackageNameChecks: true, + wantBadPackageNames: map[string]struct{}{"helpers": {}, "models": {}}, }, { name: "invalid allowlist type", @@ -104,6 +112,11 @@ func TestVarNamingRule_Configure(t *testing.T) { arguments: lint.Arguments{[]any{"ID"}, []any{"VM"}, []any{123}}, wantErr: errors.New("invalid third argument to the var-naming rule. Expecting a options of type slice, of len==1, with map, but int"), }, + { + name: "invalid third argument extraBadPackageNames", + arguments: lint.Arguments{[]any{""}, []any{""}, []any{map[string]any{"extraBadPackageNames": []int{1}}}}, + wantErr: errors.New("invalid third argument to the var-naming rule. Expecting extraBadPackageNames of type slice of strings, but []int"), + }, } for _, tt := range tests { @@ -131,6 +144,9 @@ func TestVarNamingRule_Configure(t *testing.T) { if rule.skipPackageNameChecks != tt.wantSkipPackageNameChecks { t.Errorf("unexpected skipPackageNameChecks: got = %v, want %v", rule.skipPackageNameChecks, tt.wantSkipPackageNameChecks) } + if !reflect.DeepEqual(rule.extraBadPackageNames, tt.wantBadPackageNames) { + t.Errorf("unexpected extraBadPackageNames: got = %v, want %v", rule.extraBadPackageNames, tt.wantBadPackageNames) + } }) } } diff --git a/test/var_naming_test.go b/test/var_naming_test.go index fd5d0a2..639773f 100644 --- a/test/var_naming_test.go +++ b/test/var_naming_test.go @@ -29,4 +29,15 @@ func TestVarNaming(t *testing.T) { testRule(t, "var_naming_skip_package_name_checks_true", &rule.VarNamingRule{}, &lint.RuleConfig{ Arguments: []any{[]any{}, []any{}, []any{map[string]any{"skip-package-name-checks": true}}}, }) + testRule(t, "var_naming_meaningless_package_name", &rule.VarNamingRule{}, &lint.RuleConfig{}) + testRule(t, "var_naming_meaningless_package_name", &rule.VarNamingRule{}, &lint.RuleConfig{ + Arguments: []any{[]any{}, []any{}, + []any{map[string]any{"skip-package-name-checks": false}}, + }, + }) + testRule(t, "var_naming_bad_package_name", &rule.VarNamingRule{}, &lint.RuleConfig{ + Arguments: []any{[]any{}, []any{}, + []any{map[string]any{"skip-package-name-checks": false, "extra-bad-package-names": []string{"helpers"}}}, + }, + }) } diff --git a/testdata/var_naming_bad_package_name.go b/testdata/var_naming_bad_package_name.go new file mode 100644 index 0000000..9d66653 --- /dev/null +++ b/testdata/var_naming_bad_package_name.go @@ -0,0 +1 @@ +package helpers // MATCH /avoid bad package names/ diff --git a/testdata/var_naming_meaningless_package_name.go b/testdata/var_naming_meaningless_package_name.go new file mode 100644 index 0000000..d22f6e4 --- /dev/null +++ b/testdata/var_naming_meaningless_package_name.go @@ -0,0 +1 @@ +package util // MATCH /avoid meaningless package names/