Skip to content

Commit 7708dec

Browse files
alexandearalexandear
committed
refactor: string-format implements ConfigurableRule
1 parent 9177f50 commit 7708dec

File tree

3 files changed

+196
-200
lines changed

3 files changed

+196
-200
lines changed

‎rule/string_format.go

Lines changed: 42 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -12,20 +12,24 @@ import (
1212
)
1313

1414
// StringFormatRule lints strings and/or comments according to a set of regular expressions given as Arguments
15-
type StringFormatRule struct{}
15+
type StringFormatRule struct {
16+
rules []stringFormatSubrule
17+
}
1618

1719
// Apply applies the rule to the given file.
18-
func (*StringFormatRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
20+
func (r *StringFormatRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
1921
var failures []lint.Failure
2022

2123
onFailure := func(failure lint.Failure) {
2224
failures = append(failures, failure)
2325
}
2426

25-
w := &lintStringFormatRule{onFailure: onFailure}
26-
err := w.parseArguments(arguments)
27-
if err != nil {
28-
return newInternalFailureError(err)
27+
for i := range r.rules {
28+
r.rules[i].onFailure = onFailure
29+
}
30+
31+
w := &lintStringFormatRule{
32+
rules: r.rules,
2933
}
3034

3135
ast.Walk(w, file.AST)
@@ -38,32 +42,31 @@ func (*StringFormatRule) Name() string {
3842
return "string-format"
3943
}
4044

41-
// ParseArgumentsTest is a public wrapper around w.parseArguments used for testing. Returns the error message provided to panic, or nil if no error was encountered
42-
func (*StringFormatRule) ParseArgumentsTest(arguments lint.Arguments) *string {
43-
w := lintStringFormatRule{}
44-
c := make(chan any)
45-
// Parse the arguments in a goroutine, defer a recover() call, return the error encountered (or nil if there was no error)
46-
go func() {
47-
defer func() {
48-
err := w.parseArguments(arguments)
49-
c <- err
50-
}()
51-
}()
52-
err := <-c
53-
if err != nil {
54-
e := fmt.Sprintf("%s", err)
55-
return &e
45+
// Configure validates the rule configuration, and configures the rule accordingly.
46+
//
47+
// Configuration implements the [lint.ConfigurableRule] interface.
48+
func (r *StringFormatRule) Configure(arguments lint.Arguments) error {
49+
for i, argument := range arguments {
50+
scopes, regex, negated, errorMessage, err := r.parseArgument(argument, i)
51+
if err != nil {
52+
return err
53+
}
54+
r.rules = append(r.rules, stringFormatSubrule{
55+
scopes: scopes,
56+
regexp: regex,
57+
negated: negated,
58+
errorMessage: errorMessage,
59+
})
5660
}
5761
return nil
5862
}
5963

6064
type lintStringFormatRule struct {
61-
onFailure func(lint.Failure)
62-
rules []stringFormatSubrule
65+
rules []stringFormatSubrule
6366
}
6467

6568
type stringFormatSubrule struct {
66-
parent *lintStringFormatRule
69+
onFailure func(lint.Failure)
6770
scopes stringFormatSubruleScopes
6871
regexp *regexp.Regexp
6972
negated bool
@@ -84,45 +87,28 @@ const identRegex = "[_A-Za-z][_A-Za-z0-9]*"
8487
var parseStringFormatScope = regexp.MustCompile(
8588
fmt.Sprintf("^(%s(?:\\.%s)?)(?:\\[([0-9]+)\\](?:\\.(%s))?)?$", identRegex, identRegex, identRegex))
8689

87-
func (w *lintStringFormatRule) parseArguments(arguments lint.Arguments) error {
88-
for i, argument := range arguments {
89-
scopes, regex, negated, errorMessage, err := w.parseArgument(argument, i)
90-
if err != nil {
91-
return err
92-
}
93-
w.rules = append(w.rules, stringFormatSubrule{
94-
parent: w,
95-
scopes: scopes,
96-
regexp: regex,
97-
negated: negated,
98-
errorMessage: errorMessage,
99-
})
100-
}
101-
return nil
102-
}
103-
104-
func (w *lintStringFormatRule) parseArgument(argument any, ruleNum int) (scopes stringFormatSubruleScopes, regex *regexp.Regexp, negated bool, errorMessage string, err error) {
90+
func (r *StringFormatRule) parseArgument(argument any, ruleNum int) (scopes stringFormatSubruleScopes, regex *regexp.Regexp, negated bool, errorMessage string, err error) {
10591
g, ok := argument.([]any) // Cast to generic slice first
10692
if !ok {
107-
return stringFormatSubruleScopes{}, regex, false, "", w.configError("argument is not a slice", ruleNum, 0)
93+
return stringFormatSubruleScopes{}, regex, false, "", r.configError("argument is not a slice", ruleNum, 0)
10894
}
10995
if len(g) < 2 {
110-
return stringFormatSubruleScopes{}, regex, false, "", w.configError("less than two slices found in argument, scope and regex are required", ruleNum, len(g)-1)
96+
return stringFormatSubruleScopes{}, regex, false, "", r.configError("less than two slices found in argument, scope and regex are required", ruleNum, len(g)-1)
11197
}
11298
rule := make([]string, len(g))
11399
for i, obj := range g {
114100
val, ok := obj.(string)
115101
if !ok {
116-
return stringFormatSubruleScopes{}, regex, false, "", w.configError("unexpected value, string was expected", ruleNum, i)
102+
return stringFormatSubruleScopes{}, regex, false, "", r.configError("unexpected value, string was expected", ruleNum, i)
117103
}
118104
rule[i] = val
119105
}
120106

121107
// Validate scope and regex length
122108
if rule[0] == "" {
123-
return stringFormatSubruleScopes{}, regex, false, "", w.configError("empty scope provided", ruleNum, 0)
109+
return stringFormatSubruleScopes{}, regex, false, "", r.configError("empty scope provided", ruleNum, 0)
124110
} else if len(rule[1]) < 2 {
125-
return stringFormatSubruleScopes{}, regex, false, "", w.configError("regex is too small (regexes should begin and end with '/')", ruleNum, 1)
111+
return stringFormatSubruleScopes{}, regex, false, "", r.configError("regex is too small (regexes should begin and end with '/')", ruleNum, 1)
126112
}
127113

128114
// Parse rule scopes
@@ -133,25 +119,25 @@ func (w *lintStringFormatRule) parseArgument(argument any, ruleNum int) (scopes
133119
rawScope = strings.TrimSpace(rawScope)
134120

135121
if len(rawScope) == 0 {
136-
return stringFormatSubruleScopes{}, regex, false, "", w.parseScopeError("empty scope in rule scopes:", ruleNum, 0, scopeNum)
122+
return stringFormatSubruleScopes{}, regex, false, "", r.parseScopeError("empty scope in rule scopes:", ruleNum, 0, scopeNum)
137123
}
138124

139125
scope := stringFormatSubruleScope{}
140126
matches := parseStringFormatScope.FindStringSubmatch(rawScope)
141127
if matches == nil {
142128
// The rule's scope didn't match the parsing regex at all, probably a configuration error
143-
return stringFormatSubruleScopes{}, regex, false, "", w.parseScopeError("unable to parse rule scope", ruleNum, 0, scopeNum)
129+
return stringFormatSubruleScopes{}, regex, false, "", r.parseScopeError("unable to parse rule scope", ruleNum, 0, scopeNum)
144130
} else if len(matches) != 4 {
145131
// The rule's scope matched the parsing regex, but an unexpected number of submatches was returned, probably a bug
146132
return stringFormatSubruleScopes{}, regex, false, "",
147-
w.parseScopeError(fmt.Sprintf("unexpected number of submatches when parsing scope: %d, expected 4", len(matches)), ruleNum, 0, scopeNum)
133+
r.parseScopeError(fmt.Sprintf("unexpected number of submatches when parsing scope: %d, expected 4", len(matches)), ruleNum, 0, scopeNum)
148134
}
149135
scope.funcName = matches[1]
150136
if len(matches[2]) > 0 {
151137
var err error
152138
scope.argument, err = strconv.Atoi(matches[2])
153139
if err != nil {
154-
return stringFormatSubruleScopes{}, regex, false, "", w.parseScopeError("unable to parse argument number in rule scope", ruleNum, 0, scopeNum)
140+
return stringFormatSubruleScopes{}, regex, false, "", r.parseScopeError("unable to parse argument number in rule scope", ruleNum, 0, scopeNum)
155141
}
156142
}
157143
if len(matches[3]) > 0 {
@@ -169,7 +155,7 @@ func (w *lintStringFormatRule) parseArgument(argument any, ruleNum int) (scopes
169155
}
170156
regex, errr := regexp.Compile(rule[1][offset : len(rule[1])-1])
171157
if errr != nil {
172-
return stringFormatSubruleScopes{}, regex, false, "", w.parseError(fmt.Sprintf("unable to compile %s as regexp", rule[1]), ruleNum, 1)
158+
return stringFormatSubruleScopes{}, regex, false, "", r.parseError(fmt.Sprintf("unable to compile %s as regexp", rule[1]), ruleNum, 1)
173159
}
174160

175161
// Use custom error message if provided
@@ -180,17 +166,17 @@ func (w *lintStringFormatRule) parseArgument(argument any, ruleNum int) (scopes
180166
}
181167

182168
// Report an invalid config, this is specifically the user's fault
183-
func (*lintStringFormatRule) configError(msg string, ruleNum, option int) error {
169+
func (*StringFormatRule) configError(msg string, ruleNum, option int) error {
184170
return fmt.Errorf("invalid configuration for string-format: %s [argument %d, option %d]", msg, ruleNum, option)
185171
}
186172

187173
// Report a general config parsing failure, this may be the user's fault, but it isn't known for certain
188-
func (*lintStringFormatRule) parseError(msg string, ruleNum, option int) error {
174+
func (*StringFormatRule) parseError(msg string, ruleNum, option int) error {
189175
return fmt.Errorf("failed to parse configuration for string-format: %s [argument %d, option %d]", msg, ruleNum, option)
190176
}
191177

192178
// Report a general scope config parsing failure, this may be the user's fault, but it isn't known for certain
193-
func (*lintStringFormatRule) parseScopeError(msg string, ruleNum, option, scopeNum int) error {
179+
func (*StringFormatRule) parseScopeError(msg string, ruleNum, option, scopeNum int) error {
194180
return fmt.Errorf("failed to parse configuration for string-format: %s [argument %d, option %d, scope index %d]", msg, ruleNum, option, scopeNum)
195181
}
196182

@@ -314,7 +300,7 @@ func (r *stringFormatSubrule) generateFailure(node ast.Node) {
314300
failure = fmt.Sprintf("string literal doesn't match user defined regex /%s/", r.regexp.String())
315301
}
316302

317-
r.parent.onFailure(lint.Failure{
303+
r.onFailure(lint.Failure{
318304
Confidence: 1,
319305
Failure: failure,
320306
Node: node,

‎rule/string_format_test.go

Lines changed: 154 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,154 @@
1+
package rule_test
2+
3+
import (
4+
"errors"
5+
"testing"
6+
7+
"github.com/mgechev/revive/lint"
8+
"github.com/mgechev/revive/rule"
9+
)
10+
11+
func TestStringFormatConfigure(t *testing.T) {
12+
type argumentsTest struct {
13+
name string
14+
arguments lint.Arguments
15+
wantErr error
16+
}
17+
tests := []argumentsTest{
18+
{
19+
name: "Not a Slice",
20+
arguments: lint.Arguments{
21+
"this is not a slice"},
22+
wantErr: errors.New("invalid configuration for string-format: argument is not a slice [argument 0, option 0]")},
23+
{
24+
name: "Missing Regex",
25+
arguments: lint.Arguments{
26+
[]any{
27+
"method[0]"}},
28+
wantErr: errors.New("invalid configuration for string-format: less than two slices found in argument, scope and regex are required [argument 0, option 0]")},
29+
{
30+
name: "Bad Argument Type",
31+
arguments: lint.Arguments{
32+
[]any{
33+
1}},
34+
wantErr: errors.New("invalid configuration for string-format: less than two slices found in argument, scope and regex are required [argument 0, option 0]")},
35+
{
36+
name: "Empty Scope",
37+
arguments: lint.Arguments{
38+
[]any{
39+
"",
40+
"//"}},
41+
wantErr: errors.New("invalid configuration for string-format: empty scope provided [argument 0, option 0]")},
42+
{
43+
name: "Small or Empty Regex",
44+
arguments: lint.Arguments{
45+
[]any{
46+
"method[1].a",
47+
"-"}},
48+
wantErr: errors.New("invalid configuration for string-format: regex is too small (regexes should begin and end with '/') [argument 0, option 1]")},
49+
{
50+
name: "Bad Scope",
51+
arguments: lint.Arguments{
52+
[]any{
53+
"1.a",
54+
"//"}},
55+
wantErr: errors.New("failed to parse configuration for string-format: unable to parse rule scope [argument 0, option 0, scope index 0]")},
56+
{
57+
name: "Bad Regex",
58+
arguments: lint.Arguments{
59+
[]any{
60+
"method[1].a",
61+
"/(/"}},
62+
wantErr: errors.New("failed to parse configuration for string-format: unable to compile /(/ as regexp [argument 0, option 1]")},
63+
{
64+
name: "Sample Config",
65+
arguments: lint.Arguments{
66+
[]any{
67+
"core.WriteError[1].Message", "/^([^A-Z]$)/", "must not start with a capital letter"},
68+
[]any{
69+
"fmt.Errorf[0]", "/^|[^\\.!?]$/", "must not end in punctuation"},
70+
[]any{
71+
"panic", "/^[^\\n]*$/", "must not contain line breaks"}}},
72+
{
73+
name: "Underscores in Scope",
74+
arguments: lint.Arguments{
75+
[]any{
76+
"some_pkg._some_function_name[5].some_member",
77+
"//"}}},
78+
{
79+
name: "Underscores in Multiple Scopes",
80+
arguments: lint.Arguments{
81+
[]any{
82+
"fmt.Errorf[0],core.WriteError[1].Message",
83+
"//"}}},
84+
{
85+
name: "', ' Delimiter",
86+
arguments: lint.Arguments{
87+
[]any{
88+
"abc, mt.Errorf",
89+
"//"}}},
90+
{
91+
name: "' ,' Delimiter",
92+
arguments: lint.Arguments{
93+
[]any{
94+
"abc ,mt.Errorf",
95+
"//"}}},
96+
{
97+
name: "', ' Delimiter",
98+
arguments: lint.Arguments{
99+
[]any{
100+
"abc, mt.Errorf",
101+
"//"}}},
102+
{
103+
name: "', ' Delimiter",
104+
arguments: lint.Arguments{
105+
[]any{
106+
"abc, mt.Errorf",
107+
"//"}}},
108+
{
109+
name: "Empty Middle Scope",
110+
arguments: lint.Arguments{
111+
[]any{
112+
"abc, ,mt.Errorf",
113+
"//"}},
114+
wantErr: errors.New("failed to parse configuration for string-format: empty scope in rule scopes: [argument 0, option 0, scope index 1]")},
115+
{
116+
name: "Empty First Scope",
117+
arguments: lint.Arguments{
118+
[]any{
119+
",mt.Errorf",
120+
"//"}},
121+
wantErr: errors.New("failed to parse configuration for string-format: empty scope in rule scopes: [argument 0, option 0, scope index 0]")},
122+
{
123+
name: "Bad First Scope",
124+
arguments: lint.Arguments{
125+
[]any{
126+
"1.a,fmt.Errorf[0]",
127+
"//"}},
128+
wantErr: errors.New("failed to parse configuration for string-format: unable to parse rule scope [argument 0, option 0, scope index 0]")},
129+
{
130+
name: "Bad Second Scope",
131+
arguments: lint.Arguments{
132+
[]any{
133+
"fmt.Errorf[0],1.a",
134+
"//"}},
135+
wantErr: errors.New("failed to parse configuration for string-format: unable to parse rule scope [argument 0, option 0, scope index 1]")}}
136+
137+
for _, tt := range tests {
138+
t.Run(tt.name, func(t *testing.T) {
139+
var r rule.StringFormatRule
140+
141+
err := r.Configure(tt.arguments)
142+
143+
if tt.wantErr == nil {
144+
if err != nil {
145+
t.Errorf("Configure() unexpected non-nil error %q", err)
146+
}
147+
return
148+
}
149+
if err == nil || err.Error() != tt.wantErr.Error() {
150+
t.Errorf("Configure() unexpected error: got %q, want %q", err, tt.wantErr)
151+
}
152+
})
153+
}
154+
}

0 commit comments

Comments
 (0)