diff --git a/README.md b/README.md index 793924b..baadf53 100644 --- a/README.md +++ b/README.md @@ -22,6 +22,7 @@ A larger set of interesting patterns might include: * `^fmt\.Errorf$` -- forbid Errorf in favor of using github.com/pkg/errors * `^ginkgo\.F[A-Z].*$` -- forbid ginkgo focused commands (used for debug issues) * `^spew\.Dump$` -- forbid dumping detailed data to stdout +* `^fmt\.Errorf(# please use github.com/pkg/errors)?$` -- forbid Errorf, with a custom message Note that the linter has no knowledge of what packages were actually imported, so aliased imports will match these patterns. diff --git a/forbidigo/forbidigo.go b/forbidigo/forbidigo.go index 2337404..17740fa 100644 --- a/forbidigo/forbidigo.go +++ b/forbidigo/forbidigo.go @@ -24,10 +24,15 @@ type UsedIssue struct { identifier string pattern string position token.Position + customMsg string } func (a UsedIssue) Details() string { - return fmt.Sprintf("use of `%s` forbidden by pattern `%s`", a.identifier, a.pattern) + explanation := fmt.Sprintf(` because %q`, a.customMsg) + if a.customMsg == "" { + explanation = fmt.Sprintf(" by pattern `%s`", a.pattern) + } + return fmt.Sprintf("use of `%s` forbidden", a.identifier) + explanation } func (a UsedIssue) Position() token.Position { @@ -36,13 +41,13 @@ func (a UsedIssue) Position() token.Position { func (a UsedIssue) String() string { return toString(a) } -func toString(i Issue) string { +func toString(i UsedIssue) string { return fmt.Sprintf("%s at %s", i.Details(), i.Position()) } type Linter struct { cfg config - patterns []*regexp.Regexp + patterns []*pattern } func DefaultPatterns() []string { @@ -65,13 +70,13 @@ func NewLinter(patterns []string, options ...Option) (*Linter, error) { if len(patterns) == 0 { patterns = DefaultPatterns() } - compiledPatterns := make([]*regexp.Regexp, 0, len(patterns)) - for _, p := range patterns { - re, err := regexp.Compile(p) + compiledPatterns := make([]*pattern, 0, len(patterns)) + for _, ptrn := range patterns { + p, err := parse(ptrn) if err != nil { - return nil, fmt.Errorf("unable to compile pattern `%s`: %s", p, err) + return nil, err } - compiledPatterns = append(compiledPatterns, re) + compiledPatterns = append(compiledPatterns, p) } return &Linter{ cfg: cfg, @@ -158,11 +163,12 @@ func (v *visitor) Visit(node ast.Node) ast.Visitor { return v } for _, p := range v.linter.patterns { - if p.MatchString(v.textFor(node)) && !v.permit(node) { + if p.pattern.MatchString(v.textFor(node)) && !v.permit(node) { v.issues = append(v.issues, UsedIssue{ identifier: v.textFor(node), - pattern: p.String(), + pattern: p.pattern.String(), position: v.fset.Position(node.Pos()), + customMsg: p.msg, }) } } diff --git a/forbidigo/forbidigo_test.go b/forbidigo/forbidigo_test.go index 9b370ab..d8b0f8d 100644 --- a/forbidigo/forbidigo_test.go +++ b/forbidigo/forbidigo_test.go @@ -19,6 +19,16 @@ func foo() { }`, "use of `fmt.Printf` forbidden by pattern `fmt\\.Printf` at testing.go:5:2") }) + t.Run("displays custom messages", func(t *testing.T) { + linter, _ := NewLinter([]string{`^fmt\.Printf(# a custom message)?$`}) + expectIssues(t, linter, ` +package bar + +func foo() { + fmt.Printf("here i am") +}`, "use of `fmt.Printf` forbidden because \"a custom message\" at testing.go:5:2") + }) + t.Run("it doesn't require a package on the identifier", func(t *testing.T) { linter, _ := NewLinter([]string{`Printf`}) expectIssues(t, linter, ` diff --git a/forbidigo/patterns.go b/forbidigo/patterns.go new file mode 100644 index 0000000..c236488 --- /dev/null +++ b/forbidigo/patterns.go @@ -0,0 +1,43 @@ +package forbidigo + +import ( + "fmt" + "regexp" + "regexp/syntax" + "strings" +) + +type pattern struct { + pattern *regexp.Regexp + msg string +} + +func parse(ptrn string) (*pattern, error) { + ptrnRe, err := regexp.Compile(ptrn) + if err != nil { + return nil, fmt.Errorf("unable to compile pattern `%s`: %s", ptrn, err) + } + re, err := syntax.Parse(ptrn, syntax.Perl) + if err != nil { + return nil, fmt.Errorf("unable to parse pattern `%s`: %s", ptrn, err) + } + msg := extractComment(re) + return &pattern{pattern: ptrnRe, msg: msg}, nil +} + +// Traverse the leaf submatches in the regex tree and extract a comment, if any +// is present. +func extractComment(re *syntax.Regexp) string { + for _, sub := range re.Sub { + if len(sub.Sub) > 0 { + if comment := extractComment(sub); comment != "" { + return comment + } + } + subStr := sub.String() + if strings.HasPrefix(subStr, "#") { + return strings.TrimSpace(strings.TrimPrefix(subStr, "#")) + } + } + return "" +} diff --git a/forbidigo/patterns_test.go b/forbidigo/patterns_test.go new file mode 100644 index 0000000..ce4e617 --- /dev/null +++ b/forbidigo/patterns_test.go @@ -0,0 +1,57 @@ +package forbidigo + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestParseValidPatterns(t *testing.T) { + for _, tc := range []struct { + name string + ptrn string + expectedComment string + }{ + { + name: "simple expression, no comment", + ptrn: `fmt\.Errorf`, + }, + { + name: "anchored expression, no comment", + ptrn: `^fmt\.Errorf$`, + }, + { + name: "contains multiple subexpression, with comment", + ptrn: `(f)mt\.Errorf(# a comment)?`, + expectedComment: "a comment", + }, + { + name: "simple expression with comment", + ptrn: `fmt\.Println(# Please don't use this!)?`, + expectedComment: "Please don't use this!", + }, + { + name: "deeply nested expression with comment", + ptrn: `fmt\.Println((((# Please don't use this!))))?`, + expectedComment: "Please don't use this!", + }, + { + name: "anchored expression with comment", + ptrn: `^fmt\.Println(# Please don't use this!)?$`, + expectedComment: "Please don't use this!", + }, + } { + t.Run(tc.name, func(t *testing.T) { + ptrn, err := parse(tc.ptrn) + require.Nil(t, err) + assert.Equal(t, tc.ptrn, ptrn.pattern.String()) + assert.Equal(t, tc.expectedComment, ptrn.msg) + }) + } +} + +func TestParseInvalidPattern_ReturnsError(t *testing.T) { + _, err := parse(`fmt\`) + assert.NotNil(t, err) +}