From 70fc88e0985cd3a359c073f70ba388fdc10227a0 Mon Sep 17 00:00:00 2001 From: comdiv Date: Mon, 17 Jul 2023 12:11:10 +0500 Subject: [PATCH 01/10] imporve `var-naming` - add upperCaseConst option to allow UPPER_CASED constants #851 --- rule/var-naming.go | 66 ++++++++++++++------- test/var-naming_test.go | 5 ++ testdata/var-naming_upperCaseConst-false.go | 9 +++ testdata/var-naming_upperCaseConst-true.go | 9 +++ 4 files changed, 67 insertions(+), 22 deletions(-) create mode 100644 testdata/var-naming_upperCaseConst-false.go create mode 100644 testdata/var-naming_upperCaseConst-true.go diff --git a/rule/var-naming.go b/rule/var-naming.go index fa4a18864..4093c8088 100644 --- a/rule/var-naming.go +++ b/rule/var-naming.go @@ -13,11 +13,15 @@ import ( var anyCapsRE = regexp.MustCompile(`[A-Z]`) +// regexp for constant names like `SOME_CONST`, `SOME_CONST_2`, `X123_3` (#851) +var upperCaseConstRE = regexp.MustCompile(`^[A-Z][A-Z\d]*(_[A-Z\d]+)*$`) + // VarNamingRule lints given else constructs. type VarNamingRule struct { - configured bool - whitelist []string - blacklist []string + configured bool + whitelist []string + blacklist []string + upperCaseConst bool // if true - allows to use UPPER_SOME_NAMES for constants sync.Mutex } @@ -31,6 +35,16 @@ func (r *VarNamingRule) configure(arguments lint.Arguments) { if len(arguments) >= 2 { r.blacklist = getList(arguments[1], "blacklist") } + + // try to find first argument of type map - treat it as "other options" map + for _, a := range arguments { + args, ok := a.(map[string]interface{}) + if ok { + r.upperCaseConst = fmt.Sprint(args["upperCaseConst"]) == "true" + break + } + } + r.configured = true } r.Unlock() @@ -52,6 +66,7 @@ func (r *VarNamingRule) Apply(file *lint.File, arguments lint.Arguments) []lint. onFailure: func(failure lint.Failure) { failures = append(failures, failure) }, + upperCaseConst: r.upperCaseConst, } // Package names need slightly different handling than other names. @@ -82,18 +97,18 @@ func (*VarNamingRule) Name() string { return "var-naming" } -func checkList(fl *ast.FieldList, thing string, w *lintNames) { +func (w *lintNames) checkList(fl *ast.FieldList, thing string) { if fl == nil { return } for _, f := range fl.List { for _, id := range f.Names { - check(id, thing, w) + w.check(id, thing) } } } -func check(id *ast.Ident, thing string, w *lintNames) { +func (w *lintNames) check(id *ast.Ident, thing string) { if id.Name == "_" { return } @@ -101,6 +116,12 @@ func check(id *ast.Ident, thing string, w *lintNames) { return } + // #851 upperCaseConst support + // if it's const + if thing == "const" && w.upperCaseConst && upperCaseConstRE.Match([]byte(id.Name)) { + return + } + // Handle two common styles from other languages that don't belong in Go. if len(id.Name) >= 5 && allCapsRE.MatchString(id.Name) && strings.Contains(id.Name, "_") { w.onFailure(lint.Failure{ @@ -144,11 +165,12 @@ func check(id *ast.Ident, thing string, w *lintNames) { } type lintNames struct { - file *lint.File - fileAst *ast.File - onFailure func(lint.Failure) - whitelist []string - blacklist []string + file *lint.File + fileAst *ast.File + onFailure func(lint.Failure) + whitelist []string + blacklist []string + upperCaseConst bool } func (w *lintNames) Visit(n ast.Node) ast.Visitor { @@ -159,7 +181,7 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { } for _, exp := range v.Lhs { if id, ok := exp.(*ast.Ident); ok { - check(id, "var", w) + w.check(id, "var") } } case *ast.FuncDecl: @@ -181,11 +203,11 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { // not exported in the Go API. // See https://github.com/golang/lint/issues/144. if ast.IsExported(v.Name.Name) || !isCgoExported(v) { - check(v.Name, thing, w) + w.check(v.Name, thing) } - checkList(v.Type.Params, thing+" parameter", w) - checkList(v.Type.Results, thing+" result", w) + w.checkList(v.Type.Params, thing+" parameter") + w.checkList(v.Type.Results, thing+" result") case *ast.GenDecl: if v.Tok == token.IMPORT { return w @@ -202,10 +224,10 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { for _, spec := range v.Specs { switch s := spec.(type) { case *ast.TypeSpec: - check(s.Name, thing, w) + w.check(s.Name, thing) case *ast.ValueSpec: for _, id := range s.Names { - check(id, thing, w) + w.check(id, thing) } } } @@ -217,23 +239,23 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { if !ok { // might be an embedded interface name continue } - checkList(ft.Params, "interface method parameter", w) - checkList(ft.Results, "interface method result", w) + w.checkList(ft.Params, "interface method parameter") + w.checkList(ft.Results, "interface method result") } case *ast.RangeStmt: if v.Tok == token.ASSIGN { return w } if id, ok := v.Key.(*ast.Ident); ok { - check(id, "range var", w) + w.check(id, "range var") } if id, ok := v.Value.(*ast.Ident); ok { - check(id, "range var", w) + w.check(id, "range var") } case *ast.StructType: for _, f := range v.Fields.List { for _, id := range f.Names { - check(id, "struct field", w) + w.check(id, "struct field") } } } diff --git a/test/var-naming_test.go b/test/var-naming_test.go index b6ccd411c..c481f2b1e 100644 --- a/test/var-naming_test.go +++ b/test/var-naming_test.go @@ -13,4 +13,9 @@ func TestVarNaming(t *testing.T) { }) testRule(t, "var-naming_test", &rule.VarNamingRule{}, &lint.RuleConfig{}) + + testRule(t, "var-naming_upperCaseConst-false", &rule.VarNamingRule{}, &lint.RuleConfig{}) + testRule(t, "var-naming_upperCaseConst-true", &rule.VarNamingRule{}, &lint.RuleConfig{ + Arguments: []interface{}{[]interface{}{}, []interface{}{}, map[string]interface{}{"upperCaseConst": true}}, + }) } diff --git a/testdata/var-naming_upperCaseConst-false.go b/testdata/var-naming_upperCaseConst-false.go new file mode 100644 index 000000000..3bb628e32 --- /dev/null +++ b/testdata/var-naming_upperCaseConst-false.go @@ -0,0 +1,9 @@ +// should fail if upperCaseConst = false (by default) #851 + +package fixtures + +const SOME_CONST_2 = 1 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ + +const ( + SOME_CONST_3 = 3 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ +) diff --git a/testdata/var-naming_upperCaseConst-true.go b/testdata/var-naming_upperCaseConst-true.go new file mode 100644 index 000000000..2e60c0871 --- /dev/null +++ b/testdata/var-naming_upperCaseConst-true.go @@ -0,0 +1,9 @@ +// should pass if upperCaseConst = true + +package fixtures + +const SOME_CONST_2 = 2 + +const ( + SOME_CONST_3 = 3 +) From 9aef94521363b0e172fe2d4b30e7f6fd0188c5f6 Mon Sep 17 00:00:00 2001 From: comdiv Date: Mon, 17 Jul 2023 12:26:03 +0500 Subject: [PATCH 02/10] `var-naming` fix backward/TOML compatibility #851 --- rule/var-naming.go | 20 +++++++++++++------- test/var-naming_test.go | 2 +- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/rule/var-naming.go b/rule/var-naming.go index 4093c8088..80f67a013 100644 --- a/rule/var-naming.go +++ b/rule/var-naming.go @@ -36,15 +36,21 @@ func (r *VarNamingRule) configure(arguments lint.Arguments) { r.blacklist = getList(arguments[1], "blacklist") } - // try to find first argument of type map - treat it as "other options" map - for _, a := range arguments { - args, ok := a.(map[string]interface{}) - if ok { - r.upperCaseConst = fmt.Sprint(args["upperCaseConst"]) == "true" - break + if len(arguments) >= 3 { + // not pretty code because should keep compatibility with TOML (no mixed array types) and new map parameters + asSlice, ok := arguments[2].([]interface{}) + if !ok { + panic(fmt.Sprintf("Invalid third argument to the var-naming rule. Expecting a %s of type slice, got %T", "options", arguments[2])) } + if len(asSlice) != 1 { + panic(fmt.Sprintf("Invalid third argument to the var-naming rule. Expecting a %s of type slice, of len==1, but %d", "options", len(asSlice))) + } + args, ok := asSlice[0].(map[string]interface{}) + if !ok { + panic(fmt.Sprintf("Invalid third argument to the var-naming rule. Expecting a %s of type slice, of len==1, with map, but %T", "options", asSlice[0])) + } + r.upperCaseConst = fmt.Sprint(args["upperCaseConst"]) == "true" } - r.configured = true } r.Unlock() diff --git a/test/var-naming_test.go b/test/var-naming_test.go index c481f2b1e..56108b056 100644 --- a/test/var-naming_test.go +++ b/test/var-naming_test.go @@ -16,6 +16,6 @@ func TestVarNaming(t *testing.T) { testRule(t, "var-naming_upperCaseConst-false", &rule.VarNamingRule{}, &lint.RuleConfig{}) testRule(t, "var-naming_upperCaseConst-true", &rule.VarNamingRule{}, &lint.RuleConfig{ - Arguments: []interface{}{[]interface{}{}, []interface{}{}, map[string]interface{}{"upperCaseConst": true}}, + Arguments: []interface{}{[]interface{}{}, []interface{}{}, []interface{}{map[string]interface{}{"upperCaseConst": true}}}, }) } From 7d8f482d13144bd9b12231d4fbf64b18d61ae7b4 Mon Sep 17 00:00:00 2001 From: chavacava Date: Sat, 29 Jul 2023 10:18:57 +0200 Subject: [PATCH 03/10] tiny refactoring --- rule/var-naming.go | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/rule/var-naming.go b/rule/var-naming.go index 80f67a013..2aa5ae2fd 100644 --- a/rule/var-naming.go +++ b/rule/var-naming.go @@ -38,7 +38,8 @@ func (r *VarNamingRule) configure(arguments lint.Arguments) { if len(arguments) >= 3 { // not pretty code because should keep compatibility with TOML (no mixed array types) and new map parameters - asSlice, ok := arguments[2].([]interface{}) + thirdArgument := arguments[2] + asSlice, ok := thirdArgument.([]interface{}) if !ok { panic(fmt.Sprintf("Invalid third argument to the var-naming rule. Expecting a %s of type slice, got %T", "options", arguments[2])) } @@ -124,7 +125,7 @@ func (w *lintNames) check(id *ast.Ident, thing string) { // #851 upperCaseConst support // if it's const - if thing == "const" && w.upperCaseConst && upperCaseConstRE.Match([]byte(id.Name)) { + if thing == token.CONST.String() && w.upperCaseConst && upperCaseConstRE.Match([]byte(id.Name)) { return } @@ -218,15 +219,8 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { if v.Tok == token.IMPORT { return w } - var thing string - switch v.Tok { - case token.CONST: - thing = "const" - case token.TYPE: - thing = "type" - case token.VAR: - thing = "var" - } + + thing := v.Tok.String() for _, spec := range v.Specs { switch s := spec.(type) { case *ast.TypeSpec: From 68b1193b888c7ee0624bd65fd1a4dbcce7c245a5 Mon Sep 17 00:00:00 2001 From: chavacava Date: Sat, 29 Jul 2023 10:19:24 +0200 Subject: [PATCH 04/10] add test case --- testdata/var-naming_upperCaseConst-false.go | 1 + testdata/var-naming_upperCaseConst-true.go | 1 + 2 files changed, 2 insertions(+) diff --git a/testdata/var-naming_upperCaseConst-false.go b/testdata/var-naming_upperCaseConst-false.go index 3bb628e32..b3ca342e4 100644 --- a/testdata/var-naming_upperCaseConst-false.go +++ b/testdata/var-naming_upperCaseConst-false.go @@ -6,4 +6,5 @@ const SOME_CONST_2 = 1 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ const ( SOME_CONST_3 = 3 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ + VER = 0 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ ) diff --git a/testdata/var-naming_upperCaseConst-true.go b/testdata/var-naming_upperCaseConst-true.go index 2e60c0871..272b708bc 100644 --- a/testdata/var-naming_upperCaseConst-true.go +++ b/testdata/var-naming_upperCaseConst-true.go @@ -6,4 +6,5 @@ const SOME_CONST_2 = 2 const ( SOME_CONST_3 = 3 + VER = 0 ) From 4c81181c438c9be1195aa2e9352647d2bd1083d5 Mon Sep 17 00:00:00 2001 From: comdiv Date: Mon, 31 Jul 2023 11:12:27 +0500 Subject: [PATCH 05/10] find ussue with github test --- test/utils.go | 1 + 1 file changed, 1 insertion(+) diff --git a/test/utils.go b/test/utils.go index 6ebb2052e..74ef5d22f 100644 --- a/test/utils.go +++ b/test/utils.go @@ -81,6 +81,7 @@ func assertFailures(t *testing.T, baseDir string, fi os.FileInfo, src []byte, ru failures := []lint.Failure{} for f := range ps { failures = append(failures, f) + fmt.Println("Failure:", f.Position.Start.Line, f.Position.End.Line, f.Failure, f.RuleName) } for _, in := range ins { From b271481b3382a4e12b4a4a6b04b2d0316b19ccee Mon Sep 17 00:00:00 2001 From: comdiv Date: Mon, 31 Jul 2023 11:17:04 +0500 Subject: [PATCH 06/10] try fix 2 --- .github/workflows/test.yaml | 2 +- test/utils.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index 11c4a344d..b9d4ec4e4 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -24,4 +24,4 @@ jobs: cache: true cache-dependency-path: '**/go.sum' - name: Run tests - run: go test -race ./... + run: go test -race ./... -v diff --git a/test/utils.go b/test/utils.go index 74ef5d22f..b8ba70848 100644 --- a/test/utils.go +++ b/test/utils.go @@ -81,7 +81,9 @@ func assertFailures(t *testing.T, baseDir string, fi os.FileInfo, src []byte, ru failures := []lint.Failure{} for f := range ps { failures = append(failures, f) - fmt.Println("Failure:", f.Position.Start.Line, f.Position.End.Line, f.Failure, f.RuleName) + if strings.Contains(f.GetFilename(), "var-naming_upperCaseConst-false") { + fmt.Println("Failure:", f.Position.Start.Line, f.Position.End.Line, f.Failure, f.RuleName) + } } for _, in := range ins { From 2e822134ede4aed4e7faf8a3997127cf360d2ad2 Mon Sep 17 00:00:00 2001 From: comdiv Date: Mon, 31 Jul 2023 11:23:37 +0500 Subject: [PATCH 07/10] fix rule description --- RULES_DESCRIPTIONS.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index e6fa3d02e..7a0ded74b 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -669,13 +669,17 @@ _Configuration_: N/A _Description_: This rule warns when [initialism](https://github.com/golang/go/wiki/CodeReviewComments#initialisms), [variable](https://github.com/golang/go/wiki/CodeReviewComments#variable-names) or [package](https://github.com/golang/go/wiki/CodeReviewComments#package-names) naming conventions are not followed. -_Configuration_: This rule accepts two slices of strings, a whitelist and a blacklist of initialisms. 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 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 a whitelist and second one is a blacklist of initialisms. +In map, you can add "upperCaseConst=true" parameter to allow `UPPER_CASE` for `const` +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)) Example: ```toml [rule.var-naming] - arguments = [["ID"], ["VM"]] + arguments = [["ID"], ["VM"], [{upperCaseConst=true}]] ``` ## var-declaration From 505dc39b54857789e3a32a93e7542084e95110ca Mon Sep 17 00:00:00 2001 From: comdiv Date: Mon, 31 Jul 2023 11:26:09 +0500 Subject: [PATCH 08/10] revert verbose and prints --- .github/workflows/test.yaml | 2 +- test/utils.go | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index b9d4ec4e4..11c4a344d 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -24,4 +24,4 @@ jobs: cache: true cache-dependency-path: '**/go.sum' - name: Run tests - run: go test -race ./... -v + run: go test -race ./... diff --git a/test/utils.go b/test/utils.go index b8ba70848..6ebb2052e 100644 --- a/test/utils.go +++ b/test/utils.go @@ -81,9 +81,6 @@ func assertFailures(t *testing.T, baseDir string, fi os.FileInfo, src []byte, ru failures := []lint.Failure{} for f := range ps { failures = append(failures, f) - if strings.Contains(f.GetFilename(), "var-naming_upperCaseConst-false") { - fmt.Println("Failure:", f.Position.Start.Line, f.Position.End.Line, f.Failure, f.RuleName) - } } for _, in := range ins { From 5b5e797bd11a897fbf998064eba7f5c8abd5330d Mon Sep 17 00:00:00 2001 From: comdiv Date: Mon, 31 Jul 2023 11:46:44 +0500 Subject: [PATCH 09/10] fix invalid test --- testdata/var-naming_upperCaseConst-false.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/testdata/var-naming_upperCaseConst-false.go b/testdata/var-naming_upperCaseConst-false.go index b3ca342e4..bd20c7bbe 100644 --- a/testdata/var-naming_upperCaseConst-false.go +++ b/testdata/var-naming_upperCaseConst-false.go @@ -6,5 +6,7 @@ const SOME_CONST_2 = 1 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ const ( SOME_CONST_3 = 3 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ - VER = 0 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ + // DUE TO LEGACY - names less 5 symbols without undescores are not treated as UPPER_CASE + // It's strange, but VERYLARGENAME is not linted too + VER = 0 ) From fb0b16024a274fe3c145da8e0e4e60a1f0d966c5 Mon Sep 17 00:00:00 2001 From: comdiv Date: Mon, 31 Jul 2023 11:53:16 +0500 Subject: [PATCH 10/10] fix test --- testdata/var-naming_upperCaseConst-false.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/testdata/var-naming_upperCaseConst-false.go b/testdata/var-naming_upperCaseConst-false.go index bd20c7bbe..3bb628e32 100644 --- a/testdata/var-naming_upperCaseConst-false.go +++ b/testdata/var-naming_upperCaseConst-false.go @@ -6,7 +6,4 @@ const SOME_CONST_2 = 1 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ const ( SOME_CONST_3 = 3 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/ - // DUE TO LEGACY - names less 5 symbols without undescores are not treated as UPPER_CASE - // It's strange, but VERYLARGENAME is not linted too - VER = 0 )