From ac84b09d163a392f00f3d20b8a2d3d483f6de3bf Mon Sep 17 00:00:00 2001 From: ttys3 <41882455+ttys3@users.noreply.github.com> Date: Mon, 29 Aug 2022 12:09:53 +0800 Subject: [PATCH] refactor: make config options be consistent with the cli style (#22) * refactor: make config options be consistent with the cli style * refactor: remove Config struct * refactor: postpone config process to run Signed-off-by: Timon Wong * revert some changes Signed-off-by: Timon Wong * chore: improve coverage Signed-off-by: Timon Wong * chore: fix lint Signed-off-by: Timon Wong * chore: Add a codecov file Signed-off-by: Timon Wong Signed-off-by: Timon Wong Co-authored-by: Timon Wong --- .codecov.yml | 16 ++++ config.go | 22 ------ flags.go | 75 ------------------ flags_test.go | 82 -------------------- {rules => internal/rules}/rules.go | 9 --- {rules => internal/rules}/rules_test.go | 0 sets/stringset.go => internal/sets/string.go | 26 ++++++- internal/sets/string_test.go | 61 +++++++++++++++ loggercheck.go | 49 ++++++++++-- loggercheck_test.go | 62 +++++++++++---- options.go | 14 +++- staticrules.go | 2 +- 12 files changed, 201 insertions(+), 217 deletions(-) create mode 100644 .codecov.yml delete mode 100644 config.go delete mode 100644 flags.go delete mode 100644 flags_test.go rename {rules => internal/rules}/rules.go (96%) rename {rules => internal/rules}/rules_test.go (100%) rename sets/stringset.go => internal/sets/string.go (54%) create mode 100644 internal/sets/string_test.go diff --git a/.codecov.yml b/.codecov.yml new file mode 100644 index 0000000..0956576 --- /dev/null +++ b/.codecov.yml @@ -0,0 +1,16 @@ +coverage: + range: 70..90 # green if 90+, red if 70- + status: + patch: + # coverage status for pull request diff + default: + threshold: 0.5% # allow a little drop + project: + # coverage status for whole project + default: + target: auto # use coverage of base commit as target + threshold: 0.5% # allow a little drop + +ignore: + - "plugin/**" + - "cmd/**" diff --git a/config.go b/config.go deleted file mode 100644 index ae3e414..0000000 --- a/config.go +++ /dev/null @@ -1,22 +0,0 @@ -package loggercheck - -import ( - "github.com/timonwong/loggercheck/rules" - "github.com/timonwong/loggercheck/sets" -) - -type Config struct { - Disable sets.StringSet - RulesetList rules.RulesetList -} - -func (c *Config) init(l *loggercheck) { - if c == nil { - return - } - - // Init configs from external API call (golangci-lint for example). - l.disable.StringSet = c.Disable - l.ruleFile.filename = "" - l.ruleFile.rulsetList = c.RulesetList -} diff --git a/flags.go b/flags.go deleted file mode 100644 index af84f92..0000000 --- a/flags.go +++ /dev/null @@ -1,75 +0,0 @@ -package loggercheck - -import ( - "fmt" - "os" - "strings" - - "github.com/timonwong/loggercheck/rules" - "github.com/timonwong/loggercheck/sets" -) - -type loggerCheckersFlag struct { - sets.StringSet -} - -// Set implements flag.Value interface. -func (f *loggerCheckersFlag) Set(s string) error { - s = strings.TrimSpace(s) - if s == "" { - f.StringSet = nil - return nil - } - - parts := strings.Split(s, ",") - set := sets.NewStringSet(parts...) - err := validateIgnoredLoggerFlag(set) - if err != nil { - return err - } - f.StringSet = set - return nil -} - -// String implements flag.Value interface -func (f *loggerCheckersFlag) String() string { - return strings.Join(f.List(), ",") -} - -func validateIgnoredLoggerFlag(set sets.StringSet) error { - for key := range set { - if !staticRuleList.HasName(key) { - return fmt.Errorf("unknown logger: %q", key) - } - } - - return nil -} - -type ruleFileFlag struct { - filename string - rulsetList rules.RulesetList -} - -// Set implements flag.Value interface. -func (f *ruleFileFlag) Set(filename string) error { - r, err := os.Open(filename) - if err != nil { - return err - } - defer r.Close() - - pgList, err := rules.ParseRuleFile(r) - if err != nil { - return err - } - - f.filename = filename - f.rulsetList = pgList - return nil -} - -// String implements flag.Value interface -func (f *ruleFileFlag) String() string { - return f.filename -} diff --git a/flags_test.go b/flags_test.go deleted file mode 100644 index 45358bd..0000000 --- a/flags_test.go +++ /dev/null @@ -1,82 +0,0 @@ -package loggercheck - -import ( - "flag" - "io" - "testing" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestLoggerCheckersFlag(t *testing.T) { - testCases := []struct { - name string - flagValue string - wantError string - want []string - }{ - { - name: "empty", - flagValue: "", - want: nil, - }, - { - name: "klog", - flagValue: "klog", - want: []string{"klog"}, - }, - { - name: "klog-and-logr", - flagValue: "logr,klog", - want: []string{"klog", "logr"}, - }, - { - name: "invalid-logger", - flagValue: "klog,logr,xxx", - wantError: "-ignoredloggers: unknown logger: \"xxx\"", - }, - } - - for _, tc := range testCases { - tc := tc - t.Run(tc.name, func(t *testing.T) { - t.Parallel() - - f := loggerCheckersFlag{} - fs := flag.NewFlagSet("test", flag.ContinueOnError) - fs.SetOutput(io.Discard) - fs.Var(&f, "ignoredloggers", "") - - err := fs.Parse([]string{"-ignoredloggers=" + tc.flagValue}) - if tc.wantError != "" { - assert.ErrorContains(t, err, tc.wantError) - } else { - require.NoError(t, err) - assert.Equal(t, tc.want, f.List()) - } - }) - } -} - -func TestRuleFileFlag_NoRuleFile(t *testing.T) { - f := ruleFileFlag{} - - fs := flag.NewFlagSet("test", flag.ContinueOnError) - fs.SetOutput(io.Discard) - fs.Var(&f, "rulefile", "") - - err := fs.Parse([]string{"-rulefile=testdata/xxx-not-exists-xxx.txt"}) - assert.ErrorContains(t, err, "open testdata/xxx-not-exists-xxx.txt: no such file or directory") -} - -func TestRuleFileFlag_WrongRuleFile(t *testing.T) { - f := ruleFileFlag{} - - fs := flag.NewFlagSet("test", flag.ContinueOnError) - fs.SetOutput(io.Discard) - fs.Var(&f, "rulefile", "") - - err := fs.Parse([]string{"-rulefile=testdata/wrong-rules.txt"}) - assert.ErrorContains(t, err, "error parse rule at line 2: invalid rule format") -} diff --git a/rules/rules.go b/internal/rules/rules.go similarity index 96% rename from rules/rules.go rename to internal/rules/rules.go index bc868e8..9ea76f9 100644 --- a/rules/rules.go +++ b/internal/rules/rules.go @@ -16,15 +16,6 @@ var ErrInvalidRule = errors.New("invalid rule format") type RulesetList []Ruleset -func (rl RulesetList) HasName(name string) bool { - for _, rs := range rl { - if rs.Name == name { - return true - } - } - return false -} - func (rl RulesetList) Names() []string { keys := make([]string, len(rl)) visited := make(map[string]struct{}) diff --git a/rules/rules_test.go b/internal/rules/rules_test.go similarity index 100% rename from rules/rules_test.go rename to internal/rules/rules_test.go diff --git a/sets/stringset.go b/internal/sets/string.go similarity index 54% rename from sets/stringset.go rename to internal/sets/string.go index 99239c3..daf8d57 100644 --- a/sets/stringset.go +++ b/internal/sets/string.go @@ -1,12 +1,15 @@ package sets -import "sort" +import ( + "sort" + "strings" +) type Empty struct{} type StringSet map[string]Empty -func NewStringSet(items ...string) StringSet { +func NewString(items ...string) StringSet { s := make(StringSet) s.Insert(items...) return s @@ -35,3 +38,22 @@ func (s StringSet) List() []string { sort.Strings(res) return res } + +// Set implements flag.Value interface. +func (s *StringSet) Set(v string) error { + v = strings.TrimSpace(v) + if v == "" { + *s = nil + return nil + } + + parts := strings.Split(v, ",") + set := NewString(parts...) + *s = set + return nil +} + +// String implements flag.Value interface +func (s StringSet) String() string { + return strings.Join(s.List(), ",") +} diff --git a/internal/sets/string_test.go b/internal/sets/string_test.go new file mode 100644 index 0000000..a290508 --- /dev/null +++ b/internal/sets/string_test.go @@ -0,0 +1,61 @@ +package sets + +import ( + "flag" + "io" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestString(t *testing.T) { + t.Parallel() + + set := NewString("logr", "logr", "klog") + assert.Equal(t, []string{"klog", "logr"}, set.List()) + assert.Equal(t, "klog,logr", set.String()) + assert.True(t, set.Has("logr")) + assert.True(t, set.Has("klog")) + assert.False(t, set.Has("zap")) +} + +func TestString_Flag(t *testing.T) { + testCases := []struct { + name string + flagValue string + want []string + }{ + { + name: "empty", + flagValue: "", + want: nil, + }, + { + name: "klog", + flagValue: "klog", + want: []string{"klog"}, + }, + { + name: "klog-and-logr", + flagValue: "logr,klog", + want: []string{"klog", "logr"}, + }, + } + + for _, tc := range testCases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + f := StringSet{} + fs := flag.NewFlagSet("test", flag.ContinueOnError) + fs.SetOutput(io.Discard) + fs.Var(&f, "set", "") + + err := fs.Parse([]string{"-set=" + tc.flagValue}) + require.NoError(t, err) + assert.Equal(t, tc.want, f.List()) + }) + } +} diff --git a/loggercheck.go b/loggercheck.go index 4f86c20..a0c9abe 100644 --- a/loggercheck.go +++ b/loggercheck.go @@ -5,23 +5,28 @@ import ( "fmt" "go/ast" "go/types" + "os" "strings" "golang.org/x/tools/go/analysis" "golang.org/x/tools/go/analysis/passes/inspect" "golang.org/x/tools/go/ast/inspector" "golang.org/x/tools/go/types/typeutil" + + "github.com/timonwong/loggercheck/internal/rules" + "github.com/timonwong/loggercheck/internal/sets" ) const Doc = `Checks key valur pairs for common logger libraries (logr,klog,zap).` func NewAnalyzer(opts ...Option) *analysis.Analyzer { - l := &loggercheck{} + l := &loggercheck{ + disable: sets.NewString(), + } for _, o := range opts { o(l) } - l.cfg.init(l) a := &analysis.Analyzer{ Name: "loggercheck", Doc: Doc, @@ -31,16 +36,17 @@ func NewAnalyzer(opts ...Option) *analysis.Analyzer { checkerKeys := strings.Join(staticRuleList.Names(), ",") a.Flags.Init("loggercheck", flag.ExitOnError) - a.Flags.Var(&l.ruleFile, "rulefile", "path to a file contains a list of rules.") + a.Flags.StringVar(&l.ruleFile, "rulefile", "", "path to a file contains a list of rules.") a.Flags.Var(&l.disable, "disable", fmt.Sprintf("comma-separated list of disabled logger checker (%s).", checkerKeys)) return a } type loggercheck struct { - disable loggerCheckersFlag // flag -disable - ruleFile ruleFileFlag // flag -rulefile + disable sets.StringSet // flag -disable + ruleFile string // flag -rulefile - cfg *Config // used for external integration, for example golangci-lint + customRules []string // used for external integration, for example golangci-lint + customRulesetList rules.RulesetList // populate at runtime } func (l *loggercheck) isCheckerDisabled(name string) bool { @@ -65,7 +71,7 @@ func (l *loggercheck) isValidLoggerFunc(fn *types.Func) bool { } } - customRulesetList := l.ruleFile.rulsetList + customRulesetList := l.customRulesetList for i := range customRulesetList { pg := &customRulesetList[i] if pg.Match(fn, pkg) { @@ -119,7 +125,36 @@ func (l *loggercheck) checkLoggerArguments(pass *analysis.Pass, call *ast.CallEx } } +func (l *loggercheck) processConfig() error { + if l.ruleFile != "" { + f, err := os.Open(l.ruleFile) + if err != nil { + return fmt.Errorf("failed to open rule file: %w", err) + } + defer f.Close() + + rulesetList, err := rules.ParseRuleFile(f) + if err != nil { + return fmt.Errorf("failed to parse rule file: %w", err) + } + l.customRulesetList = rulesetList + } else if len(l.customRules) > 0 { + rulesetList, err := rules.ParseRules(l.customRules) + if err != nil { + return fmt.Errorf("failed to parse rules: %w", err) + } + l.customRulesetList = rulesetList + } + + return nil +} + func (l *loggercheck) run(pass *analysis.Pass) (interface{}, error) { + err := l.processConfig() + if err != nil { + return nil, err + } + insp := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector) nodeFilter := []ast.Node{ (*ast.CallExpr)(nil), diff --git a/loggercheck_test.go b/loggercheck_test.go index 81c995b..c0d9ec3 100644 --- a/loggercheck_test.go +++ b/loggercheck_test.go @@ -1,24 +1,33 @@ package loggercheck_test import ( - "fmt" + "errors" "testing" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/tools/go/analysis/analysistest" "github.com/timonwong/loggercheck" - "github.com/timonwong/loggercheck/rules" - "github.com/timonwong/loggercheck/sets" + "github.com/timonwong/loggercheck/internal/rules" ) +type tracingTestingErrorf struct { + *testing.T +} + +func (t tracingTestingErrorf) Errorf(format string, args ...interface{}) { + t.Logf(format, args...) +} + func TestLinter(t *testing.T) { testdata := analysistest.TestData() testCases := []struct { - name string - patterns string - flags []string + name string + patterns string + flags []string + wantError error }{ { name: "all", @@ -34,8 +43,18 @@ func TestLinter(t *testing.T) { patterns: "a/customonly", flags: []string{ "-disable=logr", - fmt.Sprintf("-rulefile=%s", "testdata/custom-rules.txt"), + "-rulefile", + "testdata/custom-rules.txt", + }, + }, + { + name: "wrong-rules", + patterns: "a/customonly", + flags: []string{ + "-rulefile", + "testdata/wrong-rules.txt", }, + wantError: rules.ErrInvalidRule, }, } @@ -45,7 +64,19 @@ func TestLinter(t *testing.T) { a := loggercheck.NewAnalyzer() err := a.Flags.Parse(tc.flags) require.NoError(t, err) - analysistest.Run(t, testdata, a, tc.patterns) + + var result []*analysistest.Result + if tc.wantError != nil { + result = analysistest.Run(&tracingTestingErrorf{t}, testdata, a, tc.patterns) + } else { + result = analysistest.Run(t, testdata, a, tc.patterns) + } + require.Len(t, result, 1) + + if tc.wantError != nil { + assert.Error(t, result[0].Err) + assert.True(t, errors.Is(result[0].Err, tc.wantError)) + } }) } } @@ -53,7 +84,7 @@ func TestLinter(t *testing.T) { func TestOptions(t *testing.T) { testdata := analysistest.TestData() - pgs, err := rules.ParseRules([]string{ + customRules := []string{ "(*a/customonly.Logger).Debugw", "(*a/customonly.Logger).Infow", "(*a/customonly.Logger).Warnw", @@ -64,12 +95,7 @@ func TestOptions(t *testing.T) { "a/customonly.Warnw", "a/customonly.Errorw", "a/customonly.With", - }) - require.NoError(t, err) - customLogger := loggercheck.WithConfig(&loggercheck.Config{ - Disable: sets.NewStringSet("klog", "logr", "zap"), - RulesetList: pgs, - }) + } testCases := []struct { name string @@ -78,13 +104,15 @@ func TestOptions(t *testing.T) { { name: "disable-all-then-enable-mylogger", options: []loggercheck.Option{ - customLogger, + loggercheck.WithDisable([]string{"klog", "logr", "zap"}), + loggercheck.WithRules(customRules), }, }, { name: "ignore-logr", options: []loggercheck.Option{ - customLogger, + loggercheck.WithDisable([]string{"logr"}), + loggercheck.WithRules(customRules), }, }, } diff --git a/options.go b/options.go index d4119bb..f9571f4 100644 --- a/options.go +++ b/options.go @@ -1,9 +1,19 @@ package loggercheck +import ( + "github.com/timonwong/loggercheck/internal/sets" +) + type Option func(*loggercheck) -func WithConfig(cfg *Config) Option { +func WithDisable(disable []string) Option { + return func(l *loggercheck) { + l.disable = sets.NewString(disable...) + } +} + +func WithRules(customRules []string) Option { return func(l *loggercheck) { - l.cfg = cfg + l.customRules = customRules } } diff --git a/staticrules.go b/staticrules.go index d2d1ce9..be53b0e 100644 --- a/staticrules.go +++ b/staticrules.go @@ -3,7 +3,7 @@ package loggercheck import ( "fmt" - "github.com/timonwong/loggercheck/rules" + "github.com/timonwong/loggercheck/internal/rules" ) var staticRuleList = rules.RulesetList{