Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support custom forbid reason messages #11

Merged
merged 3 commits into from
Dec 20, 2021
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

Expand Down
26 changes: 16 additions & 10 deletions forbidigo/forbidigo.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 "%s"`, a.customMsg)
ashanbrown marked this conversation as resolved.
Show resolved Hide resolved
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 {
Expand All @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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,
})
}
}
Expand Down
10 changes: 10 additions & 0 deletions forbidigo/forbidigo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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, `
Expand Down
43 changes: 43 additions & 0 deletions forbidigo/patterns.go
Original file line number Diff line number Diff line change
@@ -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)
Copy link
Contributor Author

@craigfurman craigfurman Nov 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't spot a way in the stdlib to convert between a regexp.Regexp and a syntax.Regexp, so I think we might have to parse it twice if we want to support this feature, which is a bit of a shame.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree it's not ideal but it only happens once per expression so I'm not too concerned.

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 ""
}
57 changes: 57 additions & 0 deletions forbidigo/patterns_test.go
Original file line number Diff line number Diff line change
@@ -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)
}