Skip to content

Commit d7db174

Browse files
ldezldez
committed
minor changes
1 parent 446db3f commit d7db174

File tree

2 files changed

+132
-129
lines changed

2 files changed

+132
-129
lines changed

‎pkg/golinters/nolintlint/nolintlint.go

+114-109
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@ type BaseIssue struct {
1919
replacement *result.Replacement
2020
}
2121

22-
func (b BaseIssue) Position() token.Position { return b.position }
22+
func (b BaseIssue) Position() token.Position {
23+
return b.position
24+
}
2325

2426
func (b BaseIssue) Replacement() *result.Replacement {
2527
return b.replacement
@@ -149,135 +151,138 @@ func (l Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
149151
var issues []Issue
150152

151153
for _, node := range nodes {
152-
if file, ok := node.(*ast.File); ok {
153-
for _, c := range file.Comments {
154-
for _, comment := range c.List {
155-
if !commentPattern.MatchString(comment.Text) {
156-
continue
157-
}
154+
file, ok := node.(*ast.File)
155+
if !ok {
156+
continue
157+
}
158158

159-
// check for a space between the "//" and the directive
160-
leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text)
159+
for _, c := range file.Comments {
160+
for _, comment := range c.List {
161+
if !commentPattern.MatchString(comment.Text) {
162+
continue
163+
}
161164

162-
var leadingSpace string
163-
if len(leadingSpaceMatches) > 0 {
164-
leadingSpace = leadingSpaceMatches[1]
165-
}
165+
// check for a space between the "//" and the directive
166+
leadingSpaceMatches := leadingSpacePattern.FindStringSubmatch(comment.Text)
166167

167-
directiveWithOptionalLeadingSpace := comment.Text
168-
if len(leadingSpace) > 0 {
169-
split := strings.Split(strings.SplitN(comment.Text, ":", 2)[0], "//")
170-
directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1])
171-
}
168+
var leadingSpace string
169+
if len(leadingSpaceMatches) > 0 {
170+
leadingSpace = leadingSpaceMatches[1]
171+
}
172172

173-
pos := fset.Position(comment.Pos())
174-
end := fset.Position(comment.End())
173+
directiveWithOptionalLeadingSpace := comment.Text
174+
if len(leadingSpace) > 0 {
175+
split := strings.Split(strings.SplitN(comment.Text, ":", 2)[0], "//")
176+
directiveWithOptionalLeadingSpace = "// " + strings.TrimSpace(split[1])
177+
}
175178

176-
base := BaseIssue{
177-
fullDirective: comment.Text,
178-
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
179-
position: pos,
180-
}
179+
pos := fset.Position(comment.Pos())
180+
end := fset.Position(comment.End())
181181

182-
// check for, report and eliminate leading spaces so we can check for other issues
183-
if len(leadingSpace) > 0 {
184-
removeWhitespace := &result.Replacement{
185-
Inline: &result.InlineFix{
186-
StartCol: pos.Column + 1,
187-
Length: len(leadingSpace),
188-
NewString: "",
189-
},
190-
}
191-
if (l.needs & NeedsMachineOnly) != 0 {
192-
issue := NotMachine{BaseIssue: base}
193-
issue.BaseIssue.replacement = removeWhitespace
194-
issues = append(issues, issue)
195-
} else if len(leadingSpace) > 1 {
196-
issue := ExtraLeadingSpace{BaseIssue: base}
197-
issue.BaseIssue.replacement = removeWhitespace
198-
issue.BaseIssue.replacement.Inline.NewString = " " // assume a single space was intended
199-
issues = append(issues, issue)
200-
}
201-
}
182+
base := BaseIssue{
183+
fullDirective: comment.Text,
184+
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
185+
position: pos,
186+
}
202187

203-
fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
204-
if len(fullMatches) == 0 {
205-
issues = append(issues, ParseError{BaseIssue: base})
206-
continue
188+
// check for, report and eliminate leading spaces so we can check for other issues
189+
if len(leadingSpace) > 0 {
190+
removeWhitespace := &result.Replacement{
191+
Inline: &result.InlineFix{
192+
StartCol: pos.Column + 1,
193+
Length: len(leadingSpace),
194+
NewString: "",
195+
},
207196
}
197+
if (l.needs & NeedsMachineOnly) != 0 {
198+
issue := NotMachine{BaseIssue: base}
199+
issue.BaseIssue.replacement = removeWhitespace
200+
issues = append(issues, issue)
201+
} else if len(leadingSpace) > 1 {
202+
issue := ExtraLeadingSpace{BaseIssue: base}
203+
issue.BaseIssue.replacement = removeWhitespace
204+
issue.BaseIssue.replacement.Inline.NewString = " " // assume a single space was intended
205+
issues = append(issues, issue)
206+
}
207+
}
208208

209-
lintersText, explanation := fullMatches[1], fullMatches[2]
210-
var linters []string
211-
var linterRange []result.Range
212-
if len(lintersText) > 0 {
213-
lls := strings.Split(lintersText, ",")
214-
linters = make([]string, 0, len(lls))
215-
rangeStart := (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:")
216-
for i, ll := range lls {
217-
rangeEnd := rangeStart + len(ll)
218-
if i < len(lls)-1 {
219-
rangeEnd++ // include trailing comma
220-
}
221-
trimmedLinterName := strings.TrimSpace(ll)
222-
if trimmedLinterName != "" {
223-
linters = append(linters, trimmedLinterName)
224-
linterRange = append(linterRange, result.Range{From: rangeStart, To: rangeEnd})
225-
}
226-
rangeStart = rangeEnd
209+
fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
210+
if len(fullMatches) == 0 {
211+
issues = append(issues, ParseError{BaseIssue: base})
212+
continue
213+
}
214+
215+
lintersText, explanation := fullMatches[1], fullMatches[2]
216+
var linters []string
217+
var linterRange []result.Range
218+
if len(lintersText) > 0 {
219+
lls := strings.Split(lintersText, ",")
220+
linters = make([]string, 0, len(lls))
221+
rangeStart := (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:")
222+
for i, ll := range lls {
223+
rangeEnd := rangeStart + len(ll)
224+
if i < len(lls)-1 {
225+
rangeEnd++ // include trailing comma
227226
}
227+
trimmedLinterName := strings.TrimSpace(ll)
228+
if trimmedLinterName != "" {
229+
linters = append(linters, trimmedLinterName)
230+
linterRange = append(linterRange, result.Range{From: rangeStart, To: rangeEnd})
231+
}
232+
rangeStart = rangeEnd
228233
}
234+
}
229235

230-
if (l.needs & NeedsSpecific) != 0 {
231-
if len(linters) == 0 {
232-
issues = append(issues, NotSpecific{BaseIssue: base})
233-
}
236+
if (l.needs & NeedsSpecific) != 0 {
237+
if len(linters) == 0 {
238+
issues = append(issues, NotSpecific{BaseIssue: base})
234239
}
240+
}
235241

236-
// when detecting unused directives, we send all the directives through and filter them out in the nolint processor
237-
if (l.needs & NeedsUnused) != 0 {
238-
removeNolintCompletely := &result.Replacement{
239-
Inline: &result.InlineFix{
240-
StartCol: pos.Column - 1,
241-
Length: end.Column - pos.Column,
242-
NewString: "",
243-
},
244-
}
242+
// when detecting unused directives, we send all the directives through and filter them out in the nolint processor
243+
if (l.needs & NeedsUnused) != 0 {
244+
removeNolintCompletely := &result.Replacement{
245+
Inline: &result.InlineFix{
246+
StartCol: pos.Column - 1,
247+
Length: end.Column - pos.Column,
248+
NewString: "",
249+
},
250+
}
245251

246-
if len(linters) == 0 {
247-
issue := UnusedCandidate{BaseIssue: base}
248-
issue.replacement = removeNolintCompletely
249-
issues = append(issues, issue)
250-
} else {
251-
for _, linter := range linters {
252-
issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}
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
258-
}
259-
issues = append(issues, issue)
252+
if len(linters) == 0 {
253+
issue := UnusedCandidate{BaseIssue: base}
254+
issue.replacement = removeNolintCompletely
255+
issues = append(issues, issue)
256+
} else {
257+
for _, linter := range linters {
258+
issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}
259+
// only offer replacement if there is a single linter
260+
// because of issues around commas and the possibility of all
261+
// linters being removed
262+
if len(linters) == 1 {
263+
issue.replacement = removeNolintCompletely
260264
}
265+
issues = append(issues, issue)
261266
}
262267
}
268+
}
263269

264-
if (l.needs&NeedsExplanation) != 0 && (explanation == "" || strings.TrimSpace(explanation) == "//") {
265-
needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation
266-
// otherwise, check if we are excluding all of the mentioned linters
267-
for _, ll := range linters {
268-
if !l.excludeByLinter[ll] { // if a linter does require explanation
269-
needsExplanation = true
270-
break
271-
}
270+
if (l.needs&NeedsExplanation) != 0 && (explanation == "" || strings.TrimSpace(explanation) == "//") {
271+
needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation
272+
// otherwise, check if we are excluding all of the mentioned linters
273+
for _, ll := range linters {
274+
if !l.excludeByLinter[ll] { // if a linter does require explanation
275+
needsExplanation = true
276+
break
272277
}
278+
}
273279

274-
if needsExplanation {
275-
fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(comment.Text, "")
276-
issues = append(issues, NoExplanation{
277-
BaseIssue: base,
278-
fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
279-
})
280-
}
280+
if needsExplanation {
281+
fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(comment.Text, "")
282+
issues = append(issues, NoExplanation{
283+
BaseIssue: base,
284+
fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
285+
})
281286
}
282287
}
283288
}

‎pkg/golinters/nolintlint/nolintlint_test.go

+18-20
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,10 @@ func foo() {
4040
other() //nolintother
4141
}`,
4242
expected: []issueWithReplacement{
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},
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"},
4747
},
4848
},
4949
{
@@ -57,7 +57,7 @@ package bar
5757
//nolint:dupl
5858
func foo() {}`,
5959
expected: []issueWithReplacement{
60-
{"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", nil},
60+
{issue: "directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1"},
6161
},
6262
},
6363
{
@@ -83,8 +83,8 @@ func foo() {
8383
bad() // nolint // because
8484
}`,
8585
expected: []issueWithReplacement{
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},
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"},
8888
},
8989
},
9090
{
@@ -99,8 +99,8 @@ func foo() {
9999
}`,
100100
expected: []issueWithReplacement{
101101
{
102-
"directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
103-
&result.Replacement{
102+
issue: "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
103+
replacement: &result.Replacement{
104104
Inline: &result.InlineFix{
105105
StartCol: 10,
106106
Length: 1,
@@ -121,8 +121,8 @@ func foo() {
121121
}`,
122122
expected: []issueWithReplacement{
123123
{
124-
"directive `// nolint` should not have more than one leading space at testing.go:5:9",
125-
&result.Replacement{
124+
issue: "directive `// nolint` should not have more than one leading space at testing.go:5:9",
125+
replacement: &result.Replacement{
126126
Inline: &result.InlineFix{
127127
StartCol: 10,
128128
Length: 2,
@@ -144,7 +144,7 @@ func foo() {
144144
good() // nolint: linter1, linter2
145145
}`,
146146
expected: []issueWithReplacement{
147-
{"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", nil}, //nolint:lll // this is a string
147+
{issue: "directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9"}, //nolint:lll // this is a string
148148
},
149149
},
150150
{
@@ -168,8 +168,8 @@ func foo() {
168168
}`,
169169
expected: []issueWithReplacement{
170170
{
171-
"directive `//nolint` is unused at testing.go:5:9",
172-
&result.Replacement{
171+
issue: "directive `//nolint` is unused at testing.go:5:9",
172+
replacement: &result.Replacement{
173173
Inline: &result.InlineFix{
174174
StartCol: 8,
175175
Length: 8,
@@ -190,8 +190,8 @@ func foo() {
190190
}`,
191191
expected: []issueWithReplacement{
192192
{
193-
"directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9",
194-
&result.Replacement{
193+
issue: "directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9",
194+
replacement: &result.Replacement{
195195
Inline: &result.InlineFix{
196196
StartCol: 8,
197197
Length: 19,
@@ -212,12 +212,10 @@ func foo() {
212212
}`,
213213
expected: []issueWithReplacement{
214214
{
215-
"directive `//nolint:linter1,linter2` is unused for linter \"linter1\" at testing.go:5:9",
216-
nil,
215+
issue: "directive `//nolint:linter1,linter2` is unused for linter \"linter1\" at testing.go:5:9",
217216
},
218217
{
219-
"directive `//nolint:linter1,linter2` is unused for linter \"linter2\" at testing.go:5:9",
220-
nil,
218+
issue: "directive `//nolint:linter1,linter2` is unused for linter \"linter2\" at testing.go:5:9",
221219
},
222220
},
223221
},

0 commit comments

Comments
 (0)