Skip to content

Commit

Permalink
Simplify reports
Browse files Browse the repository at this point in the history
The current report may be a bit confusing. if there are more than one
fix suggestion, there will be multiple reports, where the first one
contain only partial fix, and only the last one contains the final fix.

To solve it, for each expression, there will be only one report,
containing all the errors in the text, and only one fix suggestion,
containing all the fixes.
  • Loading branch information
nunnatsa committed Feb 13, 2024
1 parent 1cb550c commit e574c36
Show file tree
Hide file tree
Showing 55 changed files with 1,918 additions and 1,766 deletions.
522 changes: 230 additions & 292 deletions linter/ginkgo_linter.go

Large diffs are not rendered by default.

97 changes: 97 additions & 0 deletions reports/report-builder.go
@@ -0,0 +1,97 @@
package reports

import (
"bytes"
"fmt"
"go/ast"
"go/printer"
"go/token"
"golang.org/x/tools/go/analysis"
"strings"
)

type Builder struct {
pos token.Pos
end token.Pos
oldExpr string
issues []string
fixOffer string
suggestFix bool
}

func NewBuilder(fset *token.FileSet, oldExpr ast.Expr) *Builder {
b := &Builder{
pos: oldExpr.Pos(),
end: oldExpr.End(),
oldExpr: goFmt(fset, oldExpr),
suggestFix: false,
}

return b
}

func (b *Builder) AddIssue(suggestFix bool, issue string, args ...any) {
if len(args) > 0 {
issue = fmt.Sprintf(issue, args...)
}
b.issues = append(b.issues, issue)

if suggestFix {
b.suggestFix = true
}
}

func (b *Builder) SetFixOffer(fset *token.FileSet, fixOffer ast.Expr) {
if offer := goFmt(fset, fixOffer); offer != b.oldExpr {
b.fixOffer = offer
}
}

func (b *Builder) HasReport() bool {
return len(b.issues) > 0
}

func (b *Builder) Build() analysis.Diagnostic {
diagnostic := analysis.Diagnostic{
Pos: b.pos,
Message: b.getMessage(),
}

if b.suggestFix && len(b.fixOffer) > 0 {
diagnostic.SuggestedFixes = []analysis.SuggestedFix{
{
Message: fmt.Sprintf("should replace %s with %s", b.oldExpr, b.fixOffer),
TextEdits: []analysis.TextEdit{
{
Pos: b.pos,
End: b.end,
NewText: []byte(b.fixOffer),
},
},
},
}
}

return diagnostic
}

func goFmt(fset *token.FileSet, x ast.Expr) string {
var b bytes.Buffer
_ = printer.Fprint(&b, fset, x)
return b.String()
}

func (b *Builder) getMessage() string {
sb := strings.Builder{}
sb.WriteString("ginkgo-linter: ")
if len(b.issues) > 1 {
sb.WriteString("multiple issues: ")
}
sb.WriteString(strings.Join(b.issues, "; "))

if b.suggestFix && len(b.fixOffer) != 0 {
sb.WriteString(fmt.Sprintf(". Consider using `%s` instead", b.fixOffer))
}

return sb.String()
}
2 changes: 1 addition & 1 deletion testdata/src/a/asyncconfig/asyncConfig.go
Expand Up @@ -24,6 +24,6 @@ var _ = Describe("test async suppress configuration", func() {
Eventually(slowInt()).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Should(Equal(42))
})
It("should trigger comparison waring", func() {
Eventually(slowBool()).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Should(Equal(true)) // want `ginkgo-linter: wrong boolean assertion; consider using .Eventually\(slowBool\(\)\)\.WithPolling\(time\.Millisecond \* 100\)\.WithTimeout\(time\.Second \* 2\)\.Should\(BeTrue\(\)\). instead`
Eventually(slowBool()).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Should(Equal(true)) // want `ginkgo-linter: wrong boolean assertion\. Consider using .Eventually\(slowBool\(\)\)\.WithPolling\(time\.Millisecond \* 100\)\.WithTimeout\(time\.Second \* 2\)\.Should\(BeTrue\(\)\). instead`
})
})
2 changes: 1 addition & 1 deletion testdata/src/a/asyncconfig/asyncConfig.gomega.go
Expand Up @@ -14,6 +14,6 @@ var _ = Describe("test async suppress configuration", func() {
g.Eventually(slowInt()).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Should(g.Equal(42))
})
It("should trigger comparison waring", func() {
g.Eventually(slowBool()).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Should(g.Equal(true)) // want `ginkgo-linter: wrong boolean assertion; consider using .g\.Eventually\(slowBool\(\)\)\.WithPolling\(time\.Millisecond \* 100\)\.WithTimeout\(time\.Second \* 2\)\.Should\(g\.BeTrue\(\)\). instead`
g.Eventually(slowBool()).WithPolling(time.Millisecond * 100).WithTimeout(time.Second * 2).Should(g.Equal(true)) // want `ginkgo-linter: wrong boolean assertion\. Consider using .g\.Eventually\(slowBool\(\)\)\.WithPolling\(time\.Millisecond \* 100\)\.WithTimeout\(time\.Second \* 2\)\.Should\(g\.BeTrue\(\)\). instead`
})
})
20 changes: 10 additions & 10 deletions testdata/src/a/boolean/bool.go
Expand Up @@ -10,18 +10,18 @@ var _ = Describe("boolean warnings", func() {
f := false

It("check Equal(true/false)", func() {
Expect(t).To(Equal(true)) // want `ginkgo-linter: wrong boolean assertion; consider using .Expect\(t\)\.To\(BeTrue\(\)\). instead`
Expect(f).To(Equal(false)) // want `ginkgo-linter: wrong boolean assertion; consider using .Expect\(f\)\.To\(BeFalse\(\)\). instead`
ExpectWithOffset(2, t).Should(Not(Equal(false))) // want `ginkgo-linter: wrong boolean assertion; consider using .ExpectWithOffset\(2, t\)\.Should\(BeTrue\(\)\). instead`
ExpectWithOffset(2, t).ShouldNot(Equal(false)) // want `ginkgo-linter: wrong boolean assertion; consider using .ExpectWithOffset\(2, t\)\.Should\(BeTrue\(\)\). instead`
Expect(t).WithOffset(2).ToNot(Equal(false)) // want `ginkgo-linter: wrong boolean assertion; consider using .Expect\(t\)\.WithOffset\(2\)\.To\(BeTrue\(\)\). instead`
Expect(t).NotTo(Equal(false)) // want `ginkgo-linter: wrong boolean assertion; consider using .Expect\(t\)\.To\(BeTrue\(\)\). instead`
Expect(f).WithOffset(1).ToNot(Equal(true)) // want `ginkgo-linter: wrong boolean assertion; consider using .Expect\(f\)\.WithOffset\(1\)\.ToNot\(BeTrue\(\)\). instead`
Expect(t).To(Equal(true)) // want `ginkgo-linter: wrong boolean assertion\. Consider using .Expect\(t\)\.To\(BeTrue\(\)\). instead`
Expect(f).To(Equal(false)) // want `ginkgo-linter: wrong boolean assertion\. Consider using .Expect\(f\)\.To\(BeFalse\(\)\). instead`
ExpectWithOffset(2, t).Should(Not(Equal(false))) // want `ginkgo-linter: wrong boolean assertion\. Consider using .ExpectWithOffset\(2, t\)\.Should\(BeTrue\(\)\). instead`
ExpectWithOffset(2, t).ShouldNot(Equal(false)) // want `ginkgo-linter: wrong boolean assertion\. Consider using .ExpectWithOffset\(2, t\)\.Should\(BeTrue\(\)\). instead`
Expect(t).WithOffset(2).ToNot(Equal(false)) // want `ginkgo-linter: wrong boolean assertion\. Consider using .Expect\(t\)\.WithOffset\(2\)\.To\(BeTrue\(\)\). instead`
Expect(t).NotTo(Equal(false)) // want `ginkgo-linter: wrong boolean assertion\. Consider using .Expect\(t\)\.To\(BeTrue\(\)\). instead`
Expect(f).WithOffset(1).ToNot(Equal(true)) // want `ginkgo-linter: wrong boolean assertion\. Consider using .Expect\(f\)\.WithOffset\(1\)\.ToNot\(BeTrue\(\)\). instead`
})

It("check double negative", func() {
Expect(t).ToNot(BeFalse()) // want `ginkgo-linter: avoid double negative assertion; consider using .Expect\(t\)\.To\(BeTrue\(\)\). instead`
Expect(t).NotTo(BeFalse()) // want `ginkgo-linter: avoid double negative assertion; consider using .Expect\(t\)\.To\(BeTrue\(\)\). instead`
Expect(t).WithOffset(2).ShouldNot(BeFalse()) // want `ginkgo-linter: avoid double negative assertion; consider using .Expect\(t\)\.WithOffset\(2\)\.Should\(BeTrue\(\)\). instead`
Expect(t).ToNot(BeFalse()) // want `ginkgo-linter: avoid double negative assertion\. Consider using .Expect\(t\)\.To\(BeTrue\(\)\). instead`
Expect(t).NotTo(BeFalse()) // want `ginkgo-linter: avoid double negative assertion\. Consider using .Expect\(t\)\.To\(BeTrue\(\)\). instead`
Expect(t).WithOffset(2).ShouldNot(BeFalse()) // want `ginkgo-linter: avoid double negative assertion\. Consider using .Expect\(t\)\.WithOffset\(2\)\.Should\(BeTrue\(\)\). instead`
})
})
12 changes: 6 additions & 6 deletions testdata/src/a/comparetypes/comparetypes.gomega_test.go
Expand Up @@ -43,17 +43,17 @@ var _ = Describe("compare different types", func() {
gomega.Expect(a).ShouldNot(gomega.Equal(5))
gomega.Expect(a).ShouldNot(gomega.Equal(uint(5))) // want `ginkgo-linter: use Equal with different types: Comparing int with uint; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(fint64()).ShouldNot(gomega.Equal(uint64(5))) // want `ginkgo-linter: use Equal with different types: Comparing int64 with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(fmytype()).ShouldNot(gomega.Equal(uint64(5))) // want `ginkgo-linter: use Equal with different types: Comparing a/comparetypes_test.mytype with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(fmytype()).ShouldNot(gomega.Equal(uint64(5))) // want `ginkgo-linter: use Equal with different types: Comparing a/comparetypes_test\.mytype with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).ShouldNot(gomega.Equal(int32(5))) // want `ginkgo-linter: use Equal with different types: Comparing int with int32; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).ShouldNot(gomega.Equal(uint8(5))) // want `ginkgo-linter: use Equal with different types: Comparing int with uint8; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).ShouldNot(gomega.Equal(mytype(5))) // want `ginkgo-linter: use Equal with different types: Comparing int with a/comparetypes_test.mytype; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(5).ShouldNot(gomega.Equal(mytype(5))) // want `ginkgo-linter: use Equal with different types: Comparing int with a/comparetypes_test.mytype; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).ShouldNot(gomega.Equal(mytype(5))) // want `ginkgo-linter: use Equal with different types: Comparing int with a/comparetypes_test\.mytype; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(5).ShouldNot(gomega.Equal(mytype(5))) // want `ginkgo-linter: use Equal with different types: Comparing int with a/comparetypes_test\.mytype; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`

b := int16(5)
gomega.Expect(a).ShouldNot(gomega.Equal(b)) // want `ginkgo-linter: use Equal with different types: Comparing int with int16; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`

c := mytype(5)
gomega.Expect(a).ShouldNot(gomega.Equal(c)) // want `ginkgo-linter: use Equal with different types: Comparing int with a/comparetypes_test.mytype; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).ShouldNot(gomega.Equal(c)) // want `ginkgo-linter: use Equal with different types: Comparing int with a/comparetypes_test\.mytype; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
})

It("compare interfaces", func() {
Expand All @@ -72,8 +72,8 @@ var _ = Describe("compare different types", func() {

gomega.Expect(pa).Should(gomega.HaveValue(gomega.Equal(uint64(5)))) // want `ginkgo-linter: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).Should(gomega.Not(gomega.Equal(uint64(5)))) // want `ginkgo-linter: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).Should(gomega.And(gomega.Equal(uint64(5)), gomega.Not(gomega.Equal(int32(6))))) // want `ginkgo-linter: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)` `ginkgo-linter: use Equal with different types: Comparing int with int32; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).Should(gomega.Or(gomega.Equal(uint64(5)), gomega.Not(gomega.Equal(int32(6))), gomega.Not(gomega.Equal(int8(4))))) // want `ginkgo-linter: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)` `ginkgo-linter: use Equal with different types: Comparing int with int32; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)` `ginkgo-linter: use Equal with different types: Comparing int with int8; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).Should(gomega.And(gomega.Equal(uint64(5)), gomega.Not(gomega.Equal(int32(6))))) // want `ginkgo-linter: multiple issues: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\); use Equal with different types: Comparing int with int32; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).Should(gomega.Or(gomega.Equal(uint64(5)), gomega.Not(gomega.Equal(int32(6))), gomega.Not(gomega.Equal(int8(4))))) // want `ginkgo-linter: multiple issues: use Equal with different types: Comparing int with uint64; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\); use Equal with different types: Comparing int with int32; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\); use Equal with different types: Comparing int with int8; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
gomega.Expect(a).Should(gomega.WithTransform(func(i int) int { return i + 1 }, gomega.Equal(uint(6)))) // want `ginkgo-linter: use Equal with different types: Comparing int with uint; either change the expected value type if possible, or use the BeEquivalentTo\(\) matcher, instead of Equal\(\)`
})

Expand Down

0 comments on commit e574c36

Please sign in to comment.