Skip to content

Commit de63508

Browse files
ashanbrownashanbrown
committed
Punt on multiple linters
1 parent d0f91d0 commit de63508

File tree

2 files changed

+56
-107
lines changed

2 files changed

+56
-107
lines changed

‎pkg/golinters/nolintlint/nolintlint.go

Lines changed: 8 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,7 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
195195
} else if len(leadingSpace) > 1 {
196196
issue := ExtraLeadingSpace{BaseIssue: base}
197197
issue.BaseIssue.replacement = removeWhitespace
198-
issue.BaseIssue.replacement.Inline.NewString = " " // assume a single space was intended
198+
issue.BaseIssue.replacement.Inline.NewString = " " // assume a single space was intended
199199
issues = append(issues, issue)
200200
}
201201
}
@@ -215,7 +215,7 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
215215
rangeStart := (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:")
216216
for i, ll := range lls {
217217
rangeEnd := rangeStart + len(ll)
218-
if i < len(lls) - 1 {
218+
if i < len(lls)-1 {
219219
rangeEnd++ // include trailing comma
220220
}
221221
trimmedLinterName := strings.TrimSpace(ll)
@@ -248,19 +248,14 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
248248
issue.replacement = removeNolintCompletely
249249
issues = append(issues, issue)
250250
} else {
251-
for i, linter := range linters {
251+
for _, linter := range linters {
252252
issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}
253-
replacement := removeNolintCompletely
254-
if len(linters) > 1 {
255-
replacement = &result.Replacement{
256-
Inline: &result.InlineFix{
257-
StartCol: linterRange[i].From,
258-
Length: linterRange[i].To - linterRange[i].From,
259-
NewString: "",
260-
},
261-
}
253+
// only offer replacement if there is a single linter
254+
// because of issues around commas and the possibility of all
255+
// linters being removed
256+
if len(linters) == 1 {
257+
issue.replacement = removeNolintCompletely
262258
}
263-
issue.replacement = replacement
264259
issues = append(issues, issue)
265260
}
266261
}

‎pkg/golinters/nolintlint/nolintlint_test.go

Lines changed: 48 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,10 @@ func foo() {
4040
other() //nolintother
4141
}`,
4242
expected: []issueWithReplacement{
43-
{issue: "directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1"},
44-
{issue: "directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9"},
45-
{issue: "directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9"},
46-
{issue: "directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9"},
43+
{"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1", nil},
44+
{"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9", nil},
45+
{"directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9", nil},
46+
{"directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9", nil},
4747
},
4848
},
4949
{
@@ -57,7 +57,7 @@ package bar
5757
//nolint:dupl
5858
func foo() {}`,
5959
expected: []issueWithReplacement{
60-
{issue: "directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1"},
60+
{"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", nil},
6161
},
6262
},
6363
{
@@ -83,8 +83,8 @@ func foo() {
8383
bad() // nolint // because
8484
}`,
8585
expected: []issueWithReplacement{
86-
{issue: "directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9"},
87-
{issue: "directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9"},
86+
{"directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9", nil},
87+
{"directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9", nil},
8888
},
8989
},
9090
{
@@ -97,16 +97,18 @@ func foo() {
9797
bad() // nolint
9898
good() //nolint
9999
}`,
100-
expected: []issueWithReplacement{{
101-
issue: "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
102-
replacement: &result.Replacement{
103-
Inline: &result.InlineFix{
104-
StartCol: 10,
105-
Length: 1,
106-
NewString: "",
100+
expected: []issueWithReplacement{
101+
{
102+
"directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
103+
&result.Replacement{
104+
Inline: &result.InlineFix{
105+
StartCol: 10,
106+
Length: 1,
107+
NewString: "",
108+
},
107109
},
108110
},
109-
}},
111+
},
110112
},
111113
{
112114
desc: "extra spaces in front of directive are reported",
@@ -117,16 +119,18 @@ func foo() {
117119
bad() // nolint
118120
good() // nolint
119121
}`,
120-
expected: []issueWithReplacement{{
121-
issue: "directive `// nolint` should not have more than one leading space at testing.go:5:9",
122-
replacement: &result.Replacement{
123-
Inline: &result.InlineFix{
124-
StartCol: 10,
125-
Length: 2,
126-
NewString: " ",
122+
expected: []issueWithReplacement{
123+
{
124+
"directive `// nolint` should not have more than one leading space at testing.go:5:9",
125+
&result.Replacement{
126+
Inline: &result.InlineFix{
127+
StartCol: 10,
128+
Length: 2,
129+
NewString: " ",
130+
},
127131
},
128132
},
129-
}},
133+
},
130134
},
131135
{
132136
desc: "spaces are allowed in comma-separated list of linters",
@@ -140,7 +144,7 @@ func foo() {
140144
good() // nolint: linter1, linter2
141145
}`,
142146
expected: []issueWithReplacement{
143-
{issue: "directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9"}, //nolint:lll // this is a string
147+
{"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", nil}, //nolint:lll // this is a string
144148
},
145149
},
146150
{
@@ -162,16 +166,18 @@ package bar
162166
func foo() {
163167
bad() //nolint
164168
}`,
165-
expected: []issueWithReplacement{{
166-
issue: "directive `//nolint` is unused at testing.go:5:9",
167-
replacement: &result.Replacement{
168-
Inline: &result.InlineFix{
169-
StartCol: 8,
170-
Length: 8,
171-
NewString: "",
169+
expected: []issueWithReplacement{
170+
{
171+
"directive `//nolint` is unused at testing.go:5:9",
172+
&result.Replacement{
173+
Inline: &result.InlineFix{
174+
StartCol: 8,
175+
Length: 8,
176+
NewString: "",
177+
},
172178
},
173179
},
174-
}},
180+
},
175181
},
176182
{
177183
desc: "needs unused with one specific linter generates replacement",
@@ -181,89 +187,37 @@ package bar
181187
182188
func foo() {
183189
bad() //nolint:somelinter
184-
}`,
185-
expected: []issueWithReplacement{{
186-
issue: "directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9",
187-
replacement: &result.Replacement{
188-
Inline: &result.InlineFix{
189-
StartCol: 8,
190-
Length: 19,
191-
NewString: "",
192-
},
193-
},
194-
}},
195-
},
196-
{
197-
desc: "needs unused with multiple specific linters generates a replacement for each linter",
198-
needs: NeedsUnused,
199-
contents: `
200-
package bar
201-
202-
func foo() {
203-
bad() //nolint:linter1,linter2,linter3
204190
}`,
205191
expected: []issueWithReplacement{
206192
{
207-
issue: "directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter1\" at testing.go:5:9",
208-
replacement: &result.Replacement{
209-
Inline: &result.InlineFix{
210-
StartCol: 17,
211-
Length: 8,
212-
NewString: "",
213-
},
214-
},
215-
},
216-
{
217-
issue: "directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter2\" at testing.go:5:9",
218-
replacement: &result.Replacement{
219-
Inline: &result.InlineFix{
220-
StartCol: 25,
221-
Length: 8,
222-
NewString: "",
223-
},
224-
},
225-
},
226-
{
227-
issue: "directive `//nolint:linter1,linter2,linter3` is unused for linter \"linter3\" at testing.go:5:9",
228-
replacement: &result.Replacement{
193+
"directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9",
194+
&result.Replacement{
229195
Inline: &result.InlineFix{
230-
StartCol: 33,
231-
Length: 7,
196+
StartCol: 8,
197+
Length: 19,
232198
NewString: "",
233199
},
234200
},
235201
},
236202
},
237203
},
238204
{
239-
desc: "needs unused with multiple specific linters generates a replacement preserving space after commas",
205+
desc: "needs unused with multiple specific linters does not generate replacements",
240206
needs: NeedsUnused,
241207
contents: `
242208
package bar
243209
244210
func foo() {
245-
good() //nolint:linter1, linter2
211+
bad() //nolint:linter1,linter2
246212
}`,
247213
expected: []issueWithReplacement{
248214
{
249-
issue: "directive `//nolint:linter1, linter2` is unused for linter \"linter1\" at testing.go:5:10",
250-
replacement: &result.Replacement{
251-
Inline: &result.InlineFix{
252-
StartCol: 18,
253-
Length: 8,
254-
NewString: "",
255-
},
256-
},
215+
"directive `//nolint:linter1,linter2` is unused for linter \"linter1\" at testing.go:5:9",
216+
nil,
257217
},
258218
{
259-
issue: "directive `//nolint:linter1, linter2` is unused for linter \"linter2\" at testing.go:5:10",
260-
replacement: &result.Replacement{
261-
Inline: &result.InlineFix{
262-
StartCol: 26,
263-
Length: 8,
264-
NewString: "",
265-
},
266-
},
219+
"directive `//nolint:linter1,linter2` is unused for linter \"linter2\" at testing.go:5:9",
220+
nil,
267221
},
268222
},
269223
},

0 commit comments

Comments
 (0)