From af9b92ae697c5a2c75b518b287a45c77b1bd5c19 Mon Sep 17 00:00:00 2001 From: Ludovic Fernandez Date: Sat, 23 Nov 2024 18:32:11 +0100 Subject: [PATCH] dev: simplify sort results processors (#5150) --- pkg/result/processors/sort_results.go | 211 +++++---------------- pkg/result/processors/sort_results_test.go | 151 ++++++--------- 2 files changed, 108 insertions(+), 254 deletions(-) diff --git a/pkg/result/processors/sort_results.go b/pkg/result/processors/sort_results.go index 4da73c72ea39..7eebea6315ef 100644 --- a/pkg/result/processors/sort_results.go +++ b/pkg/result/processors/sort_results.go @@ -1,10 +1,9 @@ package processors import ( - "errors" + "cmp" "fmt" "slices" - "sort" "strings" "github.com/golangci/golangci-lint/pkg/config" @@ -22,24 +21,32 @@ const ( orderNameSeverity = "severity" ) +const ( + less = iota - 1 + equal + greater +) + var _ Processor = (*SortResults)(nil) +type issueComparator func(a, b *result.Issue) int + type SortResults struct { - cmps map[string]*comparator + cmps map[string][]issueComparator cfg *config.Output } func NewSortResults(cfg *config.Config) *SortResults { return &SortResults{ - cmps: map[string]*comparator{ + cmps: map[string][]issueComparator{ // For sorting we are comparing (in next order): // file names, line numbers, position, and finally - giving up. - orderNameFile: byFileName().SetNext(byLine().SetNext(byColumn())), + orderNameFile: {byFileName, byLine, 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, } @@ -57,23 +64,21 @@ func (p SortResults) Process(issues []result.Issue) ([]result.Issue, error) { p.cfg.SortOrder = []string{orderNameFile} } - var cmps []*comparator + var cmps []issueComparator + for _, name := range p.cfg.SortOrder { c, ok := p.cmps[name] if !ok { return nil, fmt.Errorf("unsupported sort-order name %q", name) } - cmps = append(cmps, c) + cmps = append(cmps, c...) } - cmp, err := mergeComparators(cmps) - if err != nil { - return nil, err - } + comp := mergeComparators(cmps...) - sort.Slice(issues, func(i, j int) bool { - return cmp.Compare(&issues[i], &issues[j]) == less + slices.SortFunc(issues, func(a, b result.Issue) int { + return comp(&a, &b) }) return issues, nil @@ -81,147 +86,32 @@ func (p SortResults) Process(issues []result.Issue) ([]result.Issue, error) { func (SortResults) Finish() {} -type compareResult int - -const ( - less compareResult = iota - 1 - equal - greater - none -) - -func (c compareResult) isNeutral() bool { - // return true if compare result is incomparable or equal. - return c == none || c == equal -} - -func (c compareResult) String() string { - switch c { - case less: - return "less" - case equal: - return "equal" - case greater: - return "greater" - default: - return "none" - } -} - -// comparator describes how to implement compare for two "issues". -type comparator struct { - name string - compare func(a, b *result.Issue) compareResult - next *comparator -} - -func (cmp *comparator) Next() *comparator { return cmp.next } - -func (cmp *comparator) SetNext(c *comparator) *comparator { - cmp.next = c - return cmp +func byFileName(a, b *result.Issue) int { + return 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 byLine(a, b *result.Issue) int { + return numericCompare(a.Line(), b.Line()) } -func (cmp *comparator) Compare(a, b *result.Issue) compareResult { - res := cmp.compare(a, b) - if !res.isNeutral() { - return res - } - - if next := cmp.Next(); next != nil { - return next.Compare(a, b) - } - - return res +func byColumn(a, b *result.Issue) int { + return numericCompare(a.Column(), b.Column()) } -func byFileName() *comparator { - return &comparator{ - name: "byFileName", - compare: func(a, b *result.Issue) compareResult { - return compareResult(strings.Compare(a.FilePath(), b.FilePath())) - }, - } +func byLinter(a, b *result.Issue) int { + return strings.Compare(a.FromLinter, b.FromLinter) } -func byLine() *comparator { - return &comparator{ - name: "byLine", - compare: func(a, b *result.Issue) compareResult { - return numericCompare(a.Line(), b.Line()) - }, - } +func bySeverity(a, b *result.Issue) int { + return severityCompare(a.Severity, b.Severity) } -func byColumn() *comparator { - return &comparator{ - name: "byColumn", - compare: func(a, b *result.Issue) compareResult { - return numericCompare(a.Column(), b.Column()) - }, - } -} - -func byLinter() *comparator { - return &comparator{ - name: "byLinter", - compare: func(a, b *result.Issue) compareResult { - return compareResult(strings.Compare(a.FromLinter, b.FromLinter)) - }, - } -} - -func bySeverity() *comparator { - return &comparator{ - name: "bySeverity", - compare: func(a, b *result.Issue) compareResult { - return severityCompare(a.Severity, b.Severity) - }, - } -} - -func mergeComparators(cmps []*comparator) (*comparator, error) { - if len(cmps) == 0 { - return nil, errors.New("no comparator") - } - - for i := range len(cmps) - 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 { +func severityCompare(a, b string) int { // The position inside the slice define the importance (lower to higher). classic := []string{"low", "medium", "high", "warning", "error"} if slices.Contains(classic, a) && slices.Contains(classic, b) { - switch { - case slices.Index(classic, a) > slices.Index(classic, b): - return greater - case slices.Index(classic, a) < slices.Index(classic, b): - return less - default: - return equal - } + return cmp.Compare(slices.Index(classic, a), slices.Index(classic, b)) } if slices.Contains(classic, a) { @@ -232,28 +122,27 @@ func severityCompare(a, b string) compareResult { return less } - return compareResult(strings.Compare(a, b)) + return strings.Compare(a, b) } -func numericCompare(a, b int) compareResult { - var ( - isValuesInvalid = a < 0 || b < 0 - isZeroValuesBoth = a == 0 && b == 0 - isEqual = a == b - isZeroValueInA = b > 0 && a == 0 - isZeroValueInB = a > 0 && b == 0 - ) - - switch { - case isZeroValuesBoth || isEqual: +func numericCompare(a, b int) int { + // Negative values and zeros are skipped (equal) because they either invalid or "neutral" (default int value). + if a <= 0 || b <= 0 { return equal - case isValuesInvalid || isZeroValueInA || isZeroValueInB: - return none - case a > b: - return greater - case a < b: - return less } - return equal + return cmp.Compare(a, b) +} + +func mergeComparators(comps ...issueComparator) issueComparator { + return func(a, b *result.Issue) int { + for _, comp := range comps { + i := comp(a, b) + if i != equal { + return i + } + } + + return equal + } } diff --git a/pkg/result/processors/sort_results_test.go b/pkg/result/processors/sort_results_test.go index cc5b32ace543..b2aaa824daee 100644 --- a/pkg/result/processors/sort_results_test.go +++ b/pkg/result/processors/sort_results_test.go @@ -73,22 +73,23 @@ var extraSeverityIssues = []result.Issue{ type compareTestCase struct { a, b result.Issue - expected compareResult + expected int } -func testCompareValues(t *testing.T, cmp *comparator, name string, tests []compareTestCase) { - t.Parallel() - +func testCompareValues(t *testing.T, cmp issueComparator, name string, tests []compareTestCase) { for i, test := range tests { //nolint:gocritic // To ignore rangeValCopy rule t.Run(fmt.Sprintf("%s(%d)", name, i), func(t *testing.T) { - res := cmp.Compare(&test.a, &test.b) - assert.Equal(t, test.expected.String(), res.String()) + t.Parallel() + + res := cmp(&test.a, &test.b) + + assert.Equal(t, compToString(test.expected), compToString(res)) }) } } -func TestCompareByLine(t *testing.T) { - testCompareValues(t, byLine(), "Compare By Line", []compareTestCase{ +func Test_byLine(t *testing.T) { + 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 @@ -103,8 +104,8 @@ func TestCompareByLine(t *testing.T) { }) } -func TestCompareByFileName(t *testing.T) { - testCompareValues(t, byFileName(), "Compare By File Name", []compareTestCase{ +func Test_byFileName(t *testing.T) { + 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 @@ -118,23 +119,23 @@ func TestCompareByFileName(t *testing.T) { }) } -func TestCompareByColumn(t *testing.T) { - testCompareValues(t, byColumn(), "Compare By Column", []compareTestCase{ +func Test_byColumn(t *testing.T) { + 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[1], issues[2], equal}, // 70 vs zero value {issues[3], issues[3], equal}, // 60 vs 60 - {issues[2], issues[3], none}, // zero value vs 60 - {issues[2], issues[1], none}, // zero value vs 70 + {issues[2], issues[3], equal}, // zero value vs 60 + {issues[2], issues[1], equal}, // zero value vs 70 {issues[1], issues[0], less}, // 70 vs 80 {issues[1], issues[1], equal}, // 70 vs 70 - {issues[3], issues[2], none}, // vs zero value + {issues[3], issues[2], equal}, // vs zero value {issues[2], issues[2], equal}, // zero value vs zero value {issues[1], issues[1], equal}, // 70 vs 70 }) } -func TestCompareByLinter(t *testing.T) { - testCompareValues(t, byLinter(), "Compare By Linter", []compareTestCase{ +func Test_byLinter(t *testing.T) { + 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 @@ -148,8 +149,8 @@ func TestCompareByLinter(t *testing.T) { }) } -func TestCompareBySeverity(t *testing.T) { - testCompareValues(t, bySeverity(), "Compare By Severity", []compareTestCase{ +func Test_bySeverity(t *testing.T) { + 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 @@ -165,49 +166,50 @@ func TestCompareBySeverity(t *testing.T) { }) } -func TestCompareNested(t *testing.T) { - cmp := byFileName().SetNext(byLine().SetNext(byColumn())) - - testCompareValues(t, cmp, "Nested Comparing", []compareTestCase{ - {issues[1], issues[0], less}, // file_linux.go vs file_windows.go - {issues[2], issues[1], less}, // file_darwin.go vs file_linux.go - {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[3], issues[2], less}, // file_darwin.go vs file_darwin.go, 10 vs 12 - {issues[0], issues[0], equal}, // file_windows.go vs file_windows.go - {issues[2], issues[3], greater}, // file_darwin.go vs file_darwin.go, 12 vs 10 - {issues[1], issues[1], equal}, // file_linux.go vs file_linux.go - {issues[2], issues[2], equal}, // file_darwin.go vs file_darwin.go - {issues[3], issues[3], equal}, // file_darwin.go vs file_darwin.go - }) +func Test_mergeComparators(t *testing.T) { + testCompareValues(t, mergeComparators(byFileName, byLine, byColumn), "Nested Comparing", + []compareTestCase{ + {issues[1], issues[0], less}, // file_linux.go vs file_windows.go + {issues[2], issues[1], less}, // file_darwin.go vs file_linux.go + {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[3], issues[2], less}, // file_darwin.go vs file_darwin.go, 10 vs 12 + {issues[0], issues[0], equal}, // file_windows.go vs file_windows.go + {issues[2], issues[3], greater}, // file_darwin.go vs file_darwin.go, 12 vs 10 + {issues[1], issues[1], equal}, // file_linux.go vs file_linux.go + {issues[2], issues[2], equal}, // file_darwin.go vs file_darwin.go + {issues[3], issues[3], equal}, // file_darwin.go vs file_darwin.go + }, + ) } -func TestNumericCompare(t *testing.T) { +func Test_numericCompare(t *testing.T) { tests := []struct { a, b int - expected compareResult + expected int }{ {0, 0, equal}, - {0, 1, none}, - {1, 0, none}, - {1, -1, none}, - {-1, 1, none}, + {0, 1, equal}, + {1, 0, equal}, + {1, -1, equal}, + {-1, 1, equal}, {1, 1, equal}, {1, 2, less}, {2, 1, greater}, } - t.Parallel() - for i, test := range tests { t.Run(fmt.Sprintf("%s(%d)", "Numeric Compare", i), func(t *testing.T) { + t.Parallel() + res := numericCompare(test.a, test.b) - assert.Equal(t, test.expected.String(), res.String()) + + assert.Equal(t, compToString(test.expected), compToString(res)) }) } } -func TestNoSorting(t *testing.T) { +func TestSortResults_Process_noSorting(t *testing.T) { tests := make([]result.Issue, len(issues)) copy(tests, issues) @@ -218,7 +220,7 @@ func TestNoSorting(t *testing.T) { assert.Equal(t, tests, results) } -func TestSorting(t *testing.T) { +func TestSortResults_Process_Sorting(t *testing.T) { tests := make([]result.Issue, len(issues)) copy(tests, issues) @@ -231,52 +233,15 @@ func TestSorting(t *testing.T) { assert.Equal(t, []result.Issue{issues[3], issues[2], issues[1], issues[0]}, results) } -func Test_mergeComparators(t *testing.T) { - testCases := []struct { - desc string - cmps []*comparator - expected string - }{ - { - desc: "one", - cmps: []*comparator{byLinter()}, - expected: "byLinter", - }, - { - desc: "two", - cmps: []*comparator{byLinter(), byFileName()}, - expected: "byLinter > byFileName", - }, - { - desc: "all", - 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()}, - expected: "byColumn > byLine > byFileName > byLinter > bySeverity", - }, +func compToString(c int) string { + switch c { + case less: + return "less" + case greater: + return "greater" + case equal: + return "equal" + default: + return "error" } - - for _, test := range testCases { - t.Run(test.desc, func(t *testing.T) { - t.Parallel() - - cmp, err := mergeComparators(test.cmps) - require.NoError(t, err) - - assert.Equal(t, test.expected, cmp.String()) - }) - } -} - -func Test_mergeComparators_error(t *testing.T) { - _, err := mergeComparators(nil) - require.EqualError(t, err, "no comparator") }