Skip to content

Commit

Permalink
minor changes
Browse files Browse the repository at this point in the history
  • Loading branch information
@ldez
ldez committed Mar 13, 2021
1 parent 446db3f commit d7db174
Show file tree
Hide file tree
Showing 2 changed files with 132 additions and 129 deletions.
223 changes: 114 additions & 109 deletions pkg/golinters/nolintlint/nolintlint.go
View file
Expand Up @@ -19,7 +19,9 @@ type BaseIssue struct {
replacement *result.Replacement
}

func (b BaseIssue) Position() token.Position { return b.position }
func (b BaseIssue) Position() token.Position {
return b.position
}

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

for _, node := range nodes {
if file, ok := node.(*ast.File); ok {
for _, c := range file.Comments {
for _, comment := range c.List {
if !commentPattern.MatchString(comment.Text) {
continue
}
file, ok := node.(*ast.File)
if !ok {
continue
}

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

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

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

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

base := BaseIssue{
fullDirective: comment.Text,
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
position: pos,
}
pos := fset.Position(comment.Pos())
end := fset.Position(comment.End())

// check for, report and eliminate leading spaces so we can check for other issues
if len(leadingSpace) > 0 {
removeWhitespace := &result.Replacement{
Inline: &result.InlineFix{
StartCol: pos.Column + 1,
Length: len(leadingSpace),
NewString: "",
},
}
if (l.needs & NeedsMachineOnly) != 0 {
issue := NotMachine{BaseIssue: base}
issue.BaseIssue.replacement = removeWhitespace
issues = append(issues, issue)
} else if len(leadingSpace) > 1 {
issue := ExtraLeadingSpace{BaseIssue: base}
issue.BaseIssue.replacement = removeWhitespace
issue.BaseIssue.replacement.Inline.NewString = " " // assume a single space was intended
issues = append(issues, issue)
}
}
base := BaseIssue{
fullDirective: comment.Text,
directiveWithOptionalLeadingSpace: directiveWithOptionalLeadingSpace,
position: pos,
}

fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
if len(fullMatches) == 0 {
issues = append(issues, ParseError{BaseIssue: base})
continue
// check for, report and eliminate leading spaces so we can check for other issues
if len(leadingSpace) > 0 {
removeWhitespace := &result.Replacement{
Inline: &result.InlineFix{
StartCol: pos.Column + 1,
Length: len(leadingSpace),
NewString: "",
},
}
if (l.needs & NeedsMachineOnly) != 0 {
issue := NotMachine{BaseIssue: base}
issue.BaseIssue.replacement = removeWhitespace
issues = append(issues, issue)
} else if len(leadingSpace) > 1 {
issue := ExtraLeadingSpace{BaseIssue: base}
issue.BaseIssue.replacement = removeWhitespace
issue.BaseIssue.replacement.Inline.NewString = " " // assume a single space was intended
issues = append(issues, issue)
}
}

lintersText, explanation := fullMatches[1], fullMatches[2]
var linters []string
var linterRange []result.Range
if len(lintersText) > 0 {
lls := strings.Split(lintersText, ",")
linters = make([]string, 0, len(lls))
rangeStart := (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:")
for i, ll := range lls {
rangeEnd := rangeStart + len(ll)
if i < len(lls)-1 {
rangeEnd++ // include trailing comma
}
trimmedLinterName := strings.TrimSpace(ll)
if trimmedLinterName != "" {
linters = append(linters, trimmedLinterName)
linterRange = append(linterRange, result.Range{From: rangeStart, To: rangeEnd})
}
rangeStart = rangeEnd
fullMatches := fullDirectivePattern.FindStringSubmatch(comment.Text)
if len(fullMatches) == 0 {
issues = append(issues, ParseError{BaseIssue: base})
continue
}

lintersText, explanation := fullMatches[1], fullMatches[2]
var linters []string
var linterRange []result.Range
if len(lintersText) > 0 {
lls := strings.Split(lintersText, ",")
linters = make([]string, 0, len(lls))
rangeStart := (pos.Column - 1) + len("//") + len(leadingSpace) + len("nolint:")
for i, ll := range lls {
rangeEnd := rangeStart + len(ll)
if i < len(lls)-1 {
rangeEnd++ // include trailing comma
}
trimmedLinterName := strings.TrimSpace(ll)
if trimmedLinterName != "" {
linters = append(linters, trimmedLinterName)
linterRange = append(linterRange, result.Range{From: rangeStart, To: rangeEnd})
}
rangeStart = rangeEnd
}
}

if (l.needs & NeedsSpecific) != 0 {
if len(linters) == 0 {
issues = append(issues, NotSpecific{BaseIssue: base})
}
if (l.needs & NeedsSpecific) != 0 {
if len(linters) == 0 {
issues = append(issues, NotSpecific{BaseIssue: base})
}
}

// when detecting unused directives, we send all the directives through and filter them out in the nolint processor
if (l.needs & NeedsUnused) != 0 {
removeNolintCompletely := &result.Replacement{
Inline: &result.InlineFix{
StartCol: pos.Column - 1,
Length: end.Column - pos.Column,
NewString: "",
},
}
// when detecting unused directives, we send all the directives through and filter them out in the nolint processor
if (l.needs & NeedsUnused) != 0 {
removeNolintCompletely := &result.Replacement{
Inline: &result.InlineFix{
StartCol: pos.Column - 1,
Length: end.Column - pos.Column,
NewString: "",
},
}

if len(linters) == 0 {
issue := UnusedCandidate{BaseIssue: base}
issue.replacement = removeNolintCompletely
issues = append(issues, issue)
} else {
for _, linter := range linters {
issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}
// only offer replacement if there is a single linter
// because of issues around commas and the possibility of all
// linters being removed
if len(linters) == 1 {
issue.replacement = removeNolintCompletely
}
issues = append(issues, issue)
if len(linters) == 0 {
issue := UnusedCandidate{BaseIssue: base}
issue.replacement = removeNolintCompletely
issues = append(issues, issue)
} else {
for _, linter := range linters {
issue := UnusedCandidate{BaseIssue: base, ExpectedLinter: linter}
// only offer replacement if there is a single linter
// because of issues around commas and the possibility of all
// linters being removed
if len(linters) == 1 {
issue.replacement = removeNolintCompletely
}
issues = append(issues, issue)
}
}
}

if (l.needs&NeedsExplanation) != 0 && (explanation == "" || strings.TrimSpace(explanation) == "//") {
needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation
// otherwise, check if we are excluding all of the mentioned linters
for _, ll := range linters {
if !l.excludeByLinter[ll] { // if a linter does require explanation
needsExplanation = true
break
}
if (l.needs&NeedsExplanation) != 0 && (explanation == "" || strings.TrimSpace(explanation) == "//") {
needsExplanation := len(linters) == 0 // if no linters are mentioned, we must have explanation
// otherwise, check if we are excluding all of the mentioned linters
for _, ll := range linters {
if !l.excludeByLinter[ll] { // if a linter does require explanation
needsExplanation = true
break
}
}

if needsExplanation {
fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(comment.Text, "")
issues = append(issues, NoExplanation{
BaseIssue: base,
fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
})
}
if needsExplanation {
fullDirectiveWithoutExplanation := trailingBlankExplanation.ReplaceAllString(comment.Text, "")
issues = append(issues, NoExplanation{
BaseIssue: base,
fullDirectiveWithoutExplanation: fullDirectiveWithoutExplanation,
})
}
}
}
Expand Down
38 changes: 18 additions & 20 deletions pkg/golinters/nolintlint/nolintlint_test.go
View file
Expand Up @@ -40,10 +40,10 @@ func foo() {
other() //nolintother
}`,
expected: []issueWithReplacement{
{"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1", nil},
{"directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9", nil},
{"directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9", nil},
{"directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9", nil},
{issue: "directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:5:1"},
{issue: "directive `//nolint` should provide explanation such as `//nolint // this is why` at testing.go:7:9"},
{issue: "directive `//nolint //` should provide explanation such as `//nolint // this is why` at testing.go:8:9"},
{issue: "directive `//nolint // ` should provide explanation such as `//nolint // this is why` at testing.go:9:9"},
},
},
{
Expand All @@ -57,7 +57,7 @@ package bar
//nolint:dupl
func foo() {}`,
expected: []issueWithReplacement{
{"directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1", nil},
{issue: "directive `//nolint:dupl` should provide explanation such as `//nolint:dupl // this is why` at testing.go:6:1"},
},
},
{
Expand All @@ -83,8 +83,8 @@ func foo() {
bad() // nolint // because
}`,
expected: []issueWithReplacement{
{"directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9", nil},
{"directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9", nil},
{issue: "directive `//nolint` should mention specific linter such as `//nolint:my-linter` at testing.go:6:9"},
{issue: "directive `// nolint // because` should mention specific linter such as `// nolint:my-linter` at testing.go:7:9"},
},
},
{
Expand All @@ -99,8 +99,8 @@ func foo() {
}`,
expected: []issueWithReplacement{
{
"directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
&result.Replacement{
issue: "directive `// nolint` should be written without leading space as `//nolint` at testing.go:5:9",
replacement: &result.Replacement{
Inline: &result.InlineFix{
StartCol: 10,
Length: 1,
Expand All @@ -121,8 +121,8 @@ func foo() {
}`,
expected: []issueWithReplacement{
{
"directive `// nolint` should not have more than one leading space at testing.go:5:9",
&result.Replacement{
issue: "directive `// nolint` should not have more than one leading space at testing.go:5:9",
replacement: &result.Replacement{
Inline: &result.InlineFix{
StartCol: 10,
Length: 2,
Expand All @@ -144,7 +144,7 @@ func foo() {
good() // nolint: linter1, linter2
}`,
expected: []issueWithReplacement{
{"directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9", nil}, //nolint:lll // this is a string
{issue: "directive `// nolint:linter1 linter2` should match `// nolint[:<comma-separated-linters>] [// <explanation>]` at testing.go:6:9"}, //nolint:lll // this is a string
},
},
{
Expand All @@ -168,8 +168,8 @@ func foo() {
}`,
expected: []issueWithReplacement{
{
"directive `//nolint` is unused at testing.go:5:9",
&result.Replacement{
issue: "directive `//nolint` is unused at testing.go:5:9",
replacement: &result.Replacement{
Inline: &result.InlineFix{
StartCol: 8,
Length: 8,
Expand All @@ -190,8 +190,8 @@ func foo() {
}`,
expected: []issueWithReplacement{
{
"directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9",
&result.Replacement{
issue: "directive `//nolint:somelinter` is unused for linter \"somelinter\" at testing.go:5:9",
replacement: &result.Replacement{
Inline: &result.InlineFix{
StartCol: 8,
Length: 19,
Expand All @@ -212,12 +212,10 @@ func foo() {
}`,
expected: []issueWithReplacement{
{
"directive `//nolint:linter1,linter2` is unused for linter \"linter1\" at testing.go:5:9",
nil,
issue: "directive `//nolint:linter1,linter2` is unused for linter \"linter1\" at testing.go:5:9",
},
{
"directive `//nolint:linter1,linter2` is unused for linter \"linter2\" at testing.go:5:9",
nil,
issue: "directive `//nolint:linter1,linter2` is unused for linter \"linter2\" at testing.go:5:9",
},
},
},
Expand Down

0 comments on commit d7db174

Please sign in to comment.