Skip to content

Commit 9f5f957

Browse files
alexandearccoVeille
alexandear
and
ccoVeille
authored
rule: allow lowercased and kebab-cased options (#1272)
* rule: tests for Configure with named options; fix errors * rule: refactor and add tests for ifelse rules * rule: allow lowercased and kebab-cased options * test: update integration tests with lowercased params * docs: update rules descriptions * rule: simplify Configure implementation with one option * gofmt and fix lint * review: add isRuleOption, update grammar in doc, simplify regex Co-authored-by: ccoVeille <[email protected]> --------- Co-authored-by: ccoVeille <[email protected]>
1 parent 43a44af commit 9f5f957

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

54 files changed

+2529
-214
lines changed

‎RULES_DESCRIPTIONS.md

+149-60
Large diffs are not rendered by default.

‎internal/ifelse/args.go

-10
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,5 @@
11
package ifelse
22

3-
// PreserveScope is a configuration argument that prevents suggestions
4-
// that would enlarge variable scope
5-
const PreserveScope = "preserveScope"
6-
7-
// AllowJump is a configuration argument that permits early-return to
8-
// suggest introducing a new jump (return, continue, etc) statement
9-
// to reduce nesting. By default, suggestions only bring existing jumps
10-
// earlier.
11-
const AllowJump = "allowJump"
12-
133
// Args contains arguments common to the early-return, indent-error-flow
144
// and superfluous-else rules
155
type Args struct {

‎internal/ifelse/rule.go

+4-11
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import (
1010
// CheckFunc evaluates a rule against the given if-else chain and returns a message
1111
// describing the proposed refactor, along with a indicator of whether such a refactor
1212
// could be found.
13-
type CheckFunc func(Chain, Args) (string, bool)
13+
type CheckFunc func(Chain) (string, bool)
1414

1515
// Apply evaluates the given Rule on if-else chains found within the given AST,
1616
// and returns the failures.
@@ -28,16 +28,9 @@ type CheckFunc func(Chain, Args) (string, bool)
2828
//
2929
// Only the block following "bar" is linted. This is because the rules that use this function
3030
// do not presently have anything to say about earlier blocks in the chain.
31-
func Apply(check CheckFunc, node ast.Node, target Target, args lint.Arguments) []lint.Failure {
31+
func Apply(check CheckFunc, node ast.Node, target Target, args Args) []lint.Failure {
3232
v := &visitor{check: check, target: target}
33-
for _, arg := range args {
34-
switch arg {
35-
case PreserveScope:
36-
v.args.PreserveScope = true
37-
case AllowJump:
38-
v.args.AllowJump = true
39-
}
40-
}
33+
v.args = args
4134
ast.Walk(v, node)
4235
return v.failures
4336
}
@@ -126,7 +119,7 @@ func (v *visitor) visitIf(ifStmt *ast.IfStmt, chain Chain) {
126119
}
127120

128121
func (v *visitor) checkRule(ifStmt *ast.IfStmt, chain Chain) {
129-
msg, found := v.check(chain, v.args)
122+
msg, found := v.check(chain)
130123
if !found {
131124
return // passed the check
132125
}

‎rule/add_constant.go

+6-6
Original file line numberDiff line numberDiff line change
@@ -212,16 +212,16 @@ func (r *AddConstantRule) Configure(arguments lint.Arguments) error {
212212
}
213213
for k, v := range args {
214214
kind := ""
215-
switch k {
216-
case "allowFloats":
215+
switch {
216+
case isRuleOption(k, "allowFloats"):
217217
kind = kindFLOAT
218218
fallthrough
219-
case "allowInts":
219+
case isRuleOption(k, "allowInts"):
220220
if kind == "" {
221221
kind = kindINT
222222
}
223223
fallthrough
224-
case "allowStrs":
224+
case isRuleOption(k, "allowStrs"):
225225
if kind == "" {
226226
kind = kindSTRING
227227
}
@@ -230,7 +230,7 @@ func (r *AddConstantRule) Configure(arguments lint.Arguments) error {
230230
return fmt.Errorf("invalid argument to the add-constant rule, string expected. Got '%v' (%T)", v, v)
231231
}
232232
r.allowList.add(kind, list)
233-
case "maxLitCount":
233+
case isRuleOption(k, "maxLitCount"):
234234
sl, ok := v.(string)
235235
if !ok {
236236
return fmt.Errorf("invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v' (%T)", v, v)
@@ -241,7 +241,7 @@ func (r *AddConstantRule) Configure(arguments lint.Arguments) error {
241241
return fmt.Errorf("invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v'", v)
242242
}
243243
r.strLitLimit = limit
244-
case "ignoreFuncs":
244+
case isRuleOption(k, "ignoreFuncs"):
245245
excludes, ok := v.(string)
246246
if !ok {
247247
return fmt.Errorf("invalid argument to the ignoreFuncs parameter of add-constant rule, string expected. Got '%v' (%T)", v, v)

‎rule/add_constant_test.go

+193
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,193 @@
1+
package rule
2+
3+
import (
4+
"errors"
5+
"reflect"
6+
"testing"
7+
8+
"github.com/mgechev/revive/lint"
9+
)
10+
11+
func TestAddConstantRule_Configure(t *testing.T) {
12+
tests := []struct {
13+
name string
14+
arguments lint.Arguments
15+
wantErr error
16+
wantList allowList
17+
wantStrLitLimit int
18+
}{
19+
{
20+
21+
name: "no arguments",
22+
arguments: lint.Arguments{},
23+
wantErr: nil,
24+
wantList: allowList{
25+
kindINT: {},
26+
kindFLOAT: {},
27+
kindSTRING: {},
28+
},
29+
wantStrLitLimit: 2,
30+
},
31+
{
32+
name: "valid arguments",
33+
arguments: lint.Arguments{
34+
map[string]any{
35+
"allowFloats": "1.0,2.0",
36+
"allowInts": "1,2",
37+
"allowStrs": "a,b",
38+
"maxLitCount": "3",
39+
"ignoreFuncs": "fmt.Println,fmt.Printf",
40+
},
41+
},
42+
wantErr: nil,
43+
wantList: allowList{
44+
kindFLOAT: {"1.0": true, "2.0": true},
45+
kindINT: {"1": true, "2": true},
46+
kindSTRING: {"a": true, "b": true},
47+
},
48+
wantStrLitLimit: 3,
49+
},
50+
{
51+
name: "valid lowercased arguments",
52+
arguments: lint.Arguments{
53+
map[string]any{
54+
"allowfloats": "1.0,2.0",
55+
"allowints": "1,2",
56+
"allowstrs": "a,b",
57+
"maxlitcount": "3",
58+
"ignorefuncs": "fmt.Println,fmt.Printf",
59+
},
60+
},
61+
wantErr: nil,
62+
wantList: allowList{
63+
kindFLOAT: {"1.0": true, "2.0": true},
64+
kindINT: {"1": true, "2": true},
65+
kindSTRING: {"a": true, "b": true},
66+
},
67+
wantStrLitLimit: 3,
68+
},
69+
{
70+
name: "valid kebab-cased arguments",
71+
arguments: lint.Arguments{
72+
map[string]any{
73+
"allow-floats": "1.0,2.0",
74+
"allow-ints": "1,2",
75+
"allow-strs": "a,b",
76+
"max-lit-count": "3",
77+
"ignore-funcs": "fmt.Println,fmt.Printf",
78+
},
79+
},
80+
wantErr: nil,
81+
wantList: allowList{
82+
kindFLOAT: {"1.0": true, "2.0": true},
83+
kindINT: {"1": true, "2": true},
84+
kindSTRING: {"a": true, "b": true},
85+
},
86+
wantStrLitLimit: 3,
87+
},
88+
{
89+
name: "unrecognized key",
90+
arguments: lint.Arguments{
91+
map[string]any{
92+
"unknownKey": "someValue",
93+
},
94+
},
95+
wantErr: nil,
96+
wantList: allowList{
97+
kindINT: {},
98+
kindFLOAT: {},
99+
kindSTRING: {},
100+
},
101+
wantStrLitLimit: 2,
102+
},
103+
{
104+
name: "invalid argument type",
105+
arguments: lint.Arguments{
106+
"invalid_argument",
107+
},
108+
wantErr: errors.New("invalid argument to the add-constant rule, expecting a k,v map. Got string"),
109+
},
110+
{
111+
name: "invalid allowFloats value",
112+
arguments: lint.Arguments{
113+
map[string]any{
114+
"allowFloats": 123,
115+
},
116+
},
117+
wantErr: errors.New("invalid argument to the add-constant rule, string expected. Got '123' (int)"),
118+
},
119+
{
120+
name: "invalid maxLitCount value: not a string",
121+
arguments: lint.Arguments{
122+
map[string]any{
123+
"maxLitCount": 123,
124+
},
125+
},
126+
wantErr: errors.New("invalid argument to the add-constant rule, expecting string representation of an integer. Got '123' (int)"),
127+
},
128+
{
129+
name: "invalid maxLitCount value: not an int",
130+
arguments: lint.Arguments{
131+
map[string]any{
132+
"maxLitCount": "abc",
133+
},
134+
},
135+
wantErr: errors.New("invalid argument to the add-constant rule, expecting string representation of an integer. Got 'abc'"),
136+
},
137+
{
138+
name: "invalid ignoreFuncs value: not a string",
139+
arguments: lint.Arguments{
140+
map[string]any{
141+
"ignoreFuncs": 123,
142+
},
143+
},
144+
wantErr: errors.New("invalid argument to the ignoreFuncs parameter of add-constant rule, string expected. Got '123' (int)"),
145+
},
146+
{
147+
name: "invalid ignoreFuncs value: empty string",
148+
arguments: lint.Arguments{
149+
map[string]any{
150+
"ignoreFuncs": " ",
151+
},
152+
},
153+
wantErr: errors.New("invalid argument to the ignoreFuncs parameter of add-constant rule, expected regular expression must not be empty"),
154+
},
155+
{
156+
name: "invalid ignoreFuncs value: wrong regexp",
157+
arguments: lint.Arguments{
158+
map[string]any{
159+
"ignoreFuncs": "(",
160+
},
161+
},
162+
wantErr: errors.New(`invalid argument to the ignoreFuncs parameter of add-constant rule: regexp "(" does not compile: error parsing regexp: missing closing ): ` + "`(`"),
163+
},
164+
}
165+
166+
for _, tt := range tests {
167+
t.Run(tt.name, func(t *testing.T) {
168+
var rule AddConstantRule
169+
170+
err := rule.Configure(tt.arguments)
171+
172+
if tt.wantErr != nil {
173+
if err == nil {
174+
t.Errorf("unexpected error: got = nil, want = %v", tt.wantErr)
175+
return
176+
}
177+
if err.Error() != tt.wantErr.Error() {
178+
t.Errorf("unexpected error: got = %v, want = %v", err, tt.wantErr)
179+
}
180+
return
181+
}
182+
if err != nil {
183+
t.Errorf("unexpected error: got = %v, want = nil", err)
184+
}
185+
if !reflect.DeepEqual(rule.allowList, tt.wantList) {
186+
t.Errorf("unexpected allowList: got = %v, want %v", rule.allowList, tt.wantList)
187+
}
188+
if rule.strLitLimit != tt.wantStrLitLimit {
189+
t.Errorf("unexpected strLitLimit: got = %v, want %v", rule.strLitLimit, tt.wantStrLitLimit)
190+
}
191+
})
192+
}
193+
}

‎rule/context_as_argument.go

+6-8
Original file line numberDiff line numberDiff line change
@@ -74,16 +74,14 @@ func (*ContextAsArgumentRule) getAllowTypesFromArguments(args lint.Arguments) (m
7474
return nil, fmt.Errorf("invalid argument to the context-as-argument rule. Expecting a k,v map, got %T", args[0])
7575
}
7676
for k, v := range argKV {
77-
switch k {
78-
case "allowTypesBefore":
79-
typesBefore, ok := v.(string)
80-
if !ok {
81-
return nil, fmt.Errorf("invalid argument to the context-as-argument.allowTypesBefore rule. Expecting a string, got %T", v)
82-
}
83-
allowTypesBefore = append(allowTypesBefore, strings.Split(typesBefore, ",")...)
84-
default:
77+
if !isRuleOption(k, "allowTypesBefore") {
8578
return nil, fmt.Errorf("invalid argument to the context-as-argument rule. Unrecognized key %s", k)
8679
}
80+
typesBefore, ok := v.(string)
81+
if !ok {
82+
return nil, fmt.Errorf("invalid argument to the context-as-argument.allowTypesBefore rule. Expecting a string, got %T", v)
83+
}
84+
allowTypesBefore = append(allowTypesBefore, strings.Split(typesBefore, ",")...)
8785
}
8886
}
8987

0 commit comments

Comments
 (0)