From 51a963fa5bcf86488ecebd1104d82f2b162644b1 Mon Sep 17 00:00:00 2001 From: Ludovic Fernandez Date: Wed, 13 Mar 2024 21:10:08 +0100 Subject: [PATCH] chore: simplify comparators (#4499) --- pkg/result/processors/sort_results.go | 198 +++++++-------------- pkg/result/processors/sort_results_test.go | 45 ++--- 2 files changed, 89 insertions(+), 154 deletions(-) diff --git a/pkg/result/processors/sort_results.go b/pkg/result/processors/sort_results.go index 01ccc994..8e4af57e 100644 --- a/pkg/result/processors/sort_results.go +++ b/pkg/result/processors/sort_results.go @@ -25,21 +25,21 @@ const ( var _ Processor = (*SortResults)(nil) type SortResults struct { - cmps map[string][]comparator + cmps map[string]*comparator cfg *config.Output } func NewSortResults(cfg *config.Config) *SortResults { return &SortResults{ - cmps: map[string][]comparator{ + cmps: map[string]*comparator{ // For sorting we are comparing (in next order): // file names, line numbers, position, and finally - giving up. - orderNameFile: {&byFileName{}, &byLine{}, &byColumn{}}, + orderNameFile: byFileName().SetNext(byLine().SetNext(byColumn())), // For sorting we are comparing: linter name - orderNameLinter: {&byLinter{}}, + orderNameLinter: byLinter(), // For sorting we are comparing: severity - orderNameSeverity: {&bySeverity{}}, + orderNameSeverity: bySeverity(), }, cfg: &cfg.Output, } @@ -55,10 +55,10 @@ func (sr SortResults) Process(issues []result.Issue) ([]result.Issue, error) { sr.cfg.SortOrder = []string{orderNameFile} } - var cmps []comparator + var cmps []*comparator for _, name := range sr.cfg.SortOrder { if c, ok := sr.cmps[name]; ok { - cmps = append(cmps, c...) + cmps = append(cmps, c) } else { return nil, fmt.Errorf("unsupported sort-order name %q", name) } @@ -108,32 +108,30 @@ func (c compareResult) String() string { } // comparator describes how to implement compare for two "issues". -type comparator interface { - Compare(a, b *result.Issue) compareResult - Next() comparator - AddNext(comparator) comparator - fmt.Stringer +type comparator struct { + name string + compare func(a, b *result.Issue) compareResult + next *comparator } -var ( - _ comparator = (*byFileName)(nil) - _ comparator = (*byLine)(nil) - _ comparator = (*byColumn)(nil) - _ comparator = (*byLinter)(nil) - _ comparator = (*bySeverity)(nil) -) +func (cmp *comparator) Next() *comparator { return cmp.next } -type byFileName struct{ next comparator } - -func (cmp *byFileName) Next() comparator { return cmp.next } - -func (cmp *byFileName) AddNext(c comparator) comparator { +func (cmp *comparator) SetNext(c *comparator) *comparator { cmp.next = c return cmp } -func (cmp *byFileName) Compare(a, b *result.Issue) compareResult { - res := compareResult(strings.Compare(a.FilePath(), b.FilePath())) +func (cmp *comparator) String() string { + s := cmp.name + if cmp.Next() != nil { + s += " > " + cmp.Next().String() + } + + return s +} + +func (cmp *comparator) Compare(a, b *result.Issue) compareResult { + res := cmp.compare(a, b) if !res.isNeutral() { return res } @@ -145,126 +143,71 @@ func (cmp *byFileName) Compare(a, b *result.Issue) compareResult { return res } -func (cmp *byFileName) String() string { - return comparatorToString("byFileName", cmp) -} - -type byLine struct{ next comparator } - -func (cmp *byLine) Next() comparator { return cmp.next } - -func (cmp *byLine) AddNext(c comparator) comparator { - cmp.next = c - return cmp -} - -func (cmp *byLine) Compare(a, b *result.Issue) compareResult { - res := numericCompare(a.Line(), b.Line()) - if !res.isNeutral() { - return res +func byFileName() *comparator { + return &comparator{ + name: "byFileName", + compare: func(a, b *result.Issue) compareResult { + return compareResult(strings.Compare(a.FilePath(), b.FilePath())) + }, } +} - if next := cmp.Next(); next != nil { - return next.Compare(a, b) +func byLine() *comparator { + return &comparator{ + name: "byLine", + compare: func(a, b *result.Issue) compareResult { + return numericCompare(a.Line(), b.Line()) + }, } - - return res } -func (cmp *byLine) String() string { - return comparatorToString("byLine", cmp) -} - -type byColumn struct{ next comparator } - -func (cmp *byColumn) Next() comparator { return cmp.next } - -func (cmp *byColumn) AddNext(c comparator) comparator { - cmp.next = c - return cmp -} - -func (cmp *byColumn) Compare(a, b *result.Issue) compareResult { - res := numericCompare(a.Column(), b.Column()) - if !res.isNeutral() { - return res +func byColumn() *comparator { + return &comparator{ + name: "byColumn", + compare: func(a, b *result.Issue) compareResult { + return numericCompare(a.Column(), b.Column()) + }, } +} - if next := cmp.Next(); next != nil { - return next.Compare(a, b) +func byLinter() *comparator { + return &comparator{ + name: "byLinter", + compare: func(a, b *result.Issue) compareResult { + return compareResult(strings.Compare(a.FromLinter, b.FromLinter)) + }, } - - return res } -func (cmp *byColumn) String() string { - return comparatorToString("byColumn", cmp) -} - -type byLinter struct{ next comparator } - -func (cmp *byLinter) Next() comparator { return cmp.next } - -func (cmp *byLinter) AddNext(c comparator) comparator { - cmp.next = c - return cmp -} - -func (cmp *byLinter) Compare(a, b *result.Issue) compareResult { - res := compareResult(strings.Compare(a.FromLinter, b.FromLinter)) - if !res.isNeutral() { - return res +func bySeverity() *comparator { + return &comparator{ + name: "bySeverity", + compare: func(a, b *result.Issue) compareResult { + return severityCompare(a.Severity, b.Severity) + }, } - - if next := cmp.Next(); next != nil { - return next.Compare(a, b) - } - - return res } -func (cmp *byLinter) String() string { - return comparatorToString("byLinter", cmp) -} - -type bySeverity struct{ next comparator } - -func (cmp *bySeverity) Next() comparator { return cmp.next } - -func (cmp *bySeverity) AddNext(c comparator) comparator { - cmp.next = c - return cmp -} - -func (cmp *bySeverity) Compare(a, b *result.Issue) compareResult { - res := severityCompare(a.Severity, b.Severity) - if !res.isNeutral() { - return res - } - - if next := cmp.Next(); next != nil { - return next.Compare(a, b) - } - - return res -} - -func (cmp *bySeverity) String() string { - return comparatorToString("bySeverity", cmp) -} - -func mergeComparators(cmps []comparator) (comparator, error) { +func mergeComparators(cmps []*comparator) (*comparator, error) { if len(cmps) == 0 { return nil, errors.New("no comparator") } for i := 0; i < len(cmps)-1; i++ { - cmps[i].AddNext(cmps[i+1]) + findComparatorTip(cmps[i]).SetNext(cmps[i+1]) } return cmps[0], nil } +func findComparatorTip(cmp *comparator) *comparator { + if cmp.Next() != nil { + return findComparatorTip(cmp.Next()) + } + + return cmp +} + func severityCompare(a, b string) compareResult { // The position inside the slice define the importance (lower to higher). classic := []string{"low", "medium", "high", "warning", "error"} @@ -313,12 +256,3 @@ func numericCompare(a, b int) compareResult { return equal } - -func comparatorToString(name string, c comparator) string { - s := name - if c.Next() != nil { - s += " > " + c.Next().String() - } - - return s -} diff --git a/pkg/result/processors/sort_results_test.go b/pkg/result/processors/sort_results_test.go index ef73d5bb..c75b0bbe 100644 --- a/pkg/result/processors/sort_results_test.go +++ b/pkg/result/processors/sort_results_test.go @@ -76,7 +76,7 @@ type compareTestCase struct { expected compareResult } -func testCompareValues(t *testing.T, cmp comparator, name string, tests []compareTestCase) { +func testCompareValues(t *testing.T, cmp *comparator, name string, tests []compareTestCase) { t.Parallel() for i := 0; i < len(tests); i++ { @@ -89,7 +89,7 @@ func testCompareValues(t *testing.T, cmp comparator, name string, tests []compar } func TestCompareByLine(t *testing.T) { - testCompareValues(t, &byLine{}, "Compare By Line", []compareTestCase{ + testCompareValues(t, byLine(), "Compare By Line", []compareTestCase{ {issues[0], issues[1], less}, // 10 vs 11 {issues[0], issues[0], equal}, // 10 vs 10 {issues[3], issues[3], equal}, // 10 vs 10 @@ -105,7 +105,7 @@ func TestCompareByLine(t *testing.T) { } func TestCompareByFileName(t *testing.T) { //nolint:dupl - testCompareValues(t, &byFileName{}, "Compare By File Name", []compareTestCase{ + testCompareValues(t, byFileName(), "Compare By File Name", []compareTestCase{ {issues[0], issues[1], greater}, // file_windows.go vs file_linux.go {issues[1], issues[2], greater}, // file_linux.go vs file_darwin.go {issues[2], issues[3], equal}, // file_darwin.go vs file_darwin.go @@ -120,7 +120,7 @@ func TestCompareByFileName(t *testing.T) { //nolint:dupl } func TestCompareByColumn(t *testing.T) { //nolint:dupl - testCompareValues(t, &byColumn{}, "Compare By Column", []compareTestCase{ + testCompareValues(t, byColumn(), "Compare By Column", []compareTestCase{ {issues[0], issues[1], greater}, // 80 vs 70 {issues[1], issues[2], none}, // 70 vs zero value {issues[3], issues[3], equal}, // 60 vs 60 @@ -135,7 +135,7 @@ func TestCompareByColumn(t *testing.T) { //nolint:dupl } func TestCompareByLinter(t *testing.T) { //nolint:dupl - testCompareValues(t, &byLinter{}, "Compare By Linter", []compareTestCase{ + testCompareValues(t, byLinter(), "Compare By Linter", []compareTestCase{ {issues[0], issues[1], greater}, // b vs a {issues[1], issues[2], less}, // a vs c {issues[2], issues[3], equal}, // c vs c @@ -150,7 +150,7 @@ func TestCompareByLinter(t *testing.T) { //nolint:dupl } func TestCompareBySeverity(t *testing.T) { - testCompareValues(t, &bySeverity{}, "Compare By Severity", []compareTestCase{ + testCompareValues(t, bySeverity(), "Compare By Severity", []compareTestCase{ {issues[0], issues[1], greater}, // medium vs low {issues[1], issues[2], less}, // low vs high {issues[2], issues[3], equal}, // high vs high @@ -167,11 +167,7 @@ func TestCompareBySeverity(t *testing.T) { } func TestCompareNested(t *testing.T) { - var cmp = &byFileName{ - next: &byLine{ - next: &byColumn{}, - }, - } + cmp := byFileName().SetNext(byLine().SetNext(byColumn())) testCompareValues(t, cmp, "Nested Comparing", []compareTestCase{ {issues[1], issues[0], less}, // file_linux.go vs file_windows.go @@ -188,7 +184,7 @@ func TestCompareNested(t *testing.T) { } func TestNumericCompare(t *testing.T) { - var tests = []struct { + tests := []struct { a, b int expected compareResult }{ @@ -214,10 +210,10 @@ func TestNumericCompare(t *testing.T) { } func TestNoSorting(t *testing.T) { - var tests = make([]result.Issue, len(issues)) + tests := make([]result.Issue, len(issues)) copy(tests, issues) - var sr = NewSortResults(&config.Config{}) + sr := NewSortResults(&config.Config{}) results, err := sr.Process(tests) require.NoError(t, err) @@ -225,12 +221,12 @@ func TestNoSorting(t *testing.T) { } func TestSorting(t *testing.T) { - var tests = make([]result.Issue, len(issues)) + tests := make([]result.Issue, len(issues)) copy(tests, issues) - var cfg = config.Config{} + cfg := config.Config{} cfg.Output.SortResults = true - var sr = NewSortResults(&cfg) + sr := NewSortResults(&cfg) results, err := sr.Process(tests) require.NoError(t, err) @@ -240,27 +236,32 @@ func TestSorting(t *testing.T) { func Test_mergeComparators(t *testing.T) { testCases := []struct { desc string - cmps []comparator + cmps []*comparator expected string }{ { desc: "one", - cmps: []comparator{&byLinter{}}, + cmps: []*comparator{byLinter()}, expected: "byLinter", }, { desc: "two", - cmps: []comparator{&byLinter{}, &byFileName{}}, + cmps: []*comparator{byLinter(), byFileName()}, expected: "byLinter > byFileName", }, { desc: "all", - cmps: []comparator{&bySeverity{}, &byLinter{}, &byFileName{}, &byLine{}, &byColumn{}}, + cmps: []*comparator{bySeverity(), byLinter(), byFileName(), byLine(), byColumn()}, expected: "bySeverity > byLinter > byFileName > byLine > byColumn", }, + { + desc: "nested", + cmps: []*comparator{bySeverity(), byFileName().SetNext(byLine().SetNext(byColumn())), byLinter()}, + expected: "bySeverity > byFileName > byLine > byColumn > byLinter", + }, { desc: "all reverse", - cmps: []comparator{&byColumn{}, &byLine{}, &byFileName{}, &byLinter{}, &bySeverity{}}, + cmps: []*comparator{byColumn(), byLine(), byFileName(), byLinter(), bySeverity()}, expected: "byColumn > byLine > byFileName > byLinter > bySeverity", }, }