Skip to content

Commit dcc0ff2

Browse files
ldezldez
committed
refactor: simplify comparison system
1 parent b5f07ea commit dcc0ff2

File tree

2 files changed

+63
-182
lines changed

2 files changed

+63
-182
lines changed

‎pkg/result/processors/sort_results.go‎

Lines changed: 35 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@ package processors
22

33
import (
44
"cmp"
5-
"errors"
65
"fmt"
76
"slices"
87
"strings"
@@ -31,21 +30,21 @@ const (
3130
var _ Processor = (*SortResults)(nil)
3231

3332
type SortResults struct {
34-
cmps map[string]*comparator
33+
cmps map[string][]issueComparator
3534

3635
cfg *config.Output
3736
}
3837

3938
func NewSortResults(cfg *config.Config) *SortResults {
4039
return &SortResults{
41-
cmps: map[string]*comparator{
40+
cmps: map[string][]issueComparator{
4241
// For sorting we are comparing (in next order):
4342
// file names, line numbers, position, and finally - giving up.
44-
orderNameFile: byFileName().SetNext(byLine().SetNext(byColumn())),
43+
orderNameFile: {byFileName, byLine, byColumn},
4544
// For sorting we are comparing: linter name
46-
orderNameLinter: byLinter(),
45+
orderNameLinter: {byLinter},
4746
// For sorting we are comparing: severity
48-
orderNameSeverity: bySeverity(),
47+
orderNameSeverity: {bySeverity},
4948
},
5049
cfg: &cfg.Output,
5150
}
@@ -63,129 +62,48 @@ func (p SortResults) Process(issues []result.Issue) ([]result.Issue, error) {
6362
p.cfg.SortOrder = []string{orderNameFile}
6463
}
6564

66-
var cmps []*comparator
65+
var cmps []issueComparator
66+
6767
for _, name := range p.cfg.SortOrder {
6868
c, ok := p.cmps[name]
6969
if !ok {
7070
return nil, fmt.Errorf("unsupported sort-order name %q", name)
7171
}
7272

73-
cmps = append(cmps, c)
73+
cmps = append(cmps, c...)
7474
}
7575

76-
comp, err := mergeComparators(cmps)
77-
if err != nil {
78-
return nil, err
79-
}
76+
comp := mergeComparators(cmps...)
8077

8178
slices.SortFunc(issues, func(a, b result.Issue) int {
82-
return comp.Compare(&a, &b)
79+
return comp(&a, &b)
8380
})
8481

8582
return issues, nil
8683
}
8784

8885
func (SortResults) Finish() {}
8986

90-
// comparator describes how to implement compare for two "issues".
91-
type comparator struct {
92-
name string
93-
compare func(a, b *result.Issue) int
94-
next *comparator
95-
}
96-
97-
func (cp *comparator) Next() *comparator { return cp.next }
98-
99-
func (cp *comparator) SetNext(c *comparator) *comparator {
100-
cp.next = c
101-
return cp
102-
}
103-
104-
func (cp *comparator) String() string {
105-
s := cp.name
106-
if cp.Next() != nil {
107-
s += " > " + cp.Next().String()
108-
}
109-
110-
return s
111-
}
112-
113-
func (cp *comparator) Compare(a, b *result.Issue) int {
114-
res := cp.compare(a, b)
115-
if res != equal {
116-
return res
117-
}
118-
119-
if next := cp.Next(); next != nil {
120-
return next.Compare(a, b)
121-
}
122-
123-
return res
124-
}
125-
126-
func byFileName() *comparator {
127-
return &comparator{
128-
name: "byFileName",
129-
compare: func(a, b *result.Issue) int {
130-
return strings.Compare(a.FilePath(), b.FilePath())
131-
},
132-
}
133-
}
134-
135-
func byLine() *comparator {
136-
return &comparator{
137-
name: "byLine",
138-
compare: func(a, b *result.Issue) int {
139-
return numericCompare(a.Line(), b.Line())
140-
},
141-
}
142-
}
87+
type issueComparator func(a *result.Issue, b *result.Issue) int
14388

144-
func byColumn() *comparator {
145-
return &comparator{
146-
name: "byColumn",
147-
compare: func(a, b *result.Issue) int {
148-
return numericCompare(a.Column(), b.Column())
149-
},
150-
}
89+
func byFileName(a, b *result.Issue) int {
90+
return strings.Compare(a.FilePath(), b.FilePath())
15191
}
15292

153-
func byLinter() *comparator {
154-
return &comparator{
155-
name: "byLinter",
156-
compare: func(a, b *result.Issue) int {
157-
return strings.Compare(a.FromLinter, b.FromLinter)
158-
},
159-
}
93+
func byLine(a, b *result.Issue) int {
94+
return numericCompare(a.Line(), b.Line())
16095
}
16196

162-
func bySeverity() *comparator {
163-
return &comparator{
164-
name: "bySeverity",
165-
compare: func(a, b *result.Issue) int {
166-
return severityCompare(a.Severity, b.Severity)
167-
},
168-
}
97+
func byColumn(a, b *result.Issue) int {
98+
return numericCompare(a.Column(), b.Column())
16999
}
170100

171-
func mergeComparators(cmps []*comparator) (*comparator, error) {
172-
if len(cmps) == 0 {
173-
return nil, errors.New("no comparator")
174-
}
175-
176-
for i := range len(cmps) - 1 {
177-
findComparatorTip(cmps[i]).SetNext(cmps[i+1])
178-
}
179-
180-
return cmps[0], nil
101+
func byLinter(a, b *result.Issue) int {
102+
return strings.Compare(a.FromLinter, b.FromLinter)
181103
}
182104

183-
func findComparatorTip(cmp *comparator) *comparator {
184-
if cmp.Next() != nil {
185-
return findComparatorTip(cmp.Next())
186-
}
187-
188-
return cmp
105+
func bySeverity(a, b *result.Issue) int {
106+
return severityCompare(a.Severity, b.Severity)
189107
}
190108

191109
func severityCompare(a, b string) int {
@@ -208,10 +126,23 @@ func severityCompare(a, b string) int {
208126
}
209127

210128
func numericCompare(a, b int) int {
211-
// Negative value and 0 are skipped because they either "neutral" (default value) or "invalid.
129+
// Negative values and zeros are skipped (equal) because they either invalid or "neutral" (default int value).
212130
if a <= 0 || b <= 0 {
213131
return equal
214132
}
215133

216134
return cmp.Compare(a, b)
217135
}
136+
137+
func mergeComparators(comps ...issueComparator) issueComparator {
138+
return func(a *result.Issue, b *result.Issue) int {
139+
for _, comp := range comps {
140+
i := comp(a, b)
141+
if i != equal {
142+
return i
143+
}
144+
}
145+
146+
return equal
147+
}
148+
}

‎pkg/result/processors/sort_results_test.go‎

Lines changed: 28 additions & 78 deletions
Original file line numberDiff line numberDiff line change
@@ -76,19 +76,19 @@ type compareTestCase struct {
7676
expected int
7777
}
7878

79-
func testCompareValues(t *testing.T, cmp *comparator, name string, tests []compareTestCase) {
79+
func testCompareValues(t *testing.T, cmp issueComparator, name string, tests []compareTestCase) {
8080
t.Parallel()
8181

8282
for i, test := range tests { //nolint:gocritic // To ignore rangeValCopy rule
8383
t.Run(fmt.Sprintf("%s(%d)", name, i), func(t *testing.T) {
84-
res := cmp.Compare(&test.a, &test.b)
84+
res := cmp(&test.a, &test.b)
8585
assert.Equal(t, compToString(test.expected), compToString(res))
8686
})
8787
}
8888
}
8989

90-
func TestCompareByLine(t *testing.T) {
91-
testCompareValues(t, byLine(), "Compare By Line", []compareTestCase{
90+
func Test_byLine(t *testing.T) {
91+
testCompareValues(t, byLine, "Compare By Line", []compareTestCase{
9292
{issues[0], issues[1], less}, // 10 vs 11
9393
{issues[0], issues[0], equal}, // 10 vs 10
9494
{issues[3], issues[3], equal}, // 10 vs 10
@@ -103,8 +103,8 @@ func TestCompareByLine(t *testing.T) {
103103
})
104104
}
105105

106-
func TestCompareByFileName(t *testing.T) {
107-
testCompareValues(t, byFileName(), "Compare By File Name", []compareTestCase{
106+
func Test_byFileName(t *testing.T) {
107+
testCompareValues(t, byFileName, "Compare By File Name", []compareTestCase{
108108
{issues[0], issues[1], greater}, // file_windows.go vs file_linux.go
109109
{issues[1], issues[2], greater}, // file_linux.go vs file_darwin.go
110110
{issues[2], issues[3], equal}, // file_darwin.go vs file_darwin.go
@@ -118,8 +118,8 @@ func TestCompareByFileName(t *testing.T) {
118118
})
119119
}
120120

121-
func TestCompareByColumn(t *testing.T) {
122-
testCompareValues(t, byColumn(), "Compare By Column", []compareTestCase{
121+
func Test_byColumn(t *testing.T) {
122+
testCompareValues(t, byColumn, "Compare By Column", []compareTestCase{
123123
{issues[0], issues[1], greater}, // 80 vs 70
124124
{issues[1], issues[2], equal}, // 70 vs zero value
125125
{issues[3], issues[3], equal}, // 60 vs 60
@@ -133,8 +133,8 @@ func TestCompareByColumn(t *testing.T) {
133133
})
134134
}
135135

136-
func TestCompareByLinter(t *testing.T) {
137-
testCompareValues(t, byLinter(), "Compare By Linter", []compareTestCase{
136+
func Test_byLinter(t *testing.T) {
137+
testCompareValues(t, byLinter, "Compare By Linter", []compareTestCase{
138138
{issues[0], issues[1], greater}, // b vs a
139139
{issues[1], issues[2], less}, // a vs c
140140
{issues[2], issues[3], equal}, // c vs c
@@ -148,8 +148,8 @@ func TestCompareByLinter(t *testing.T) {
148148
})
149149
}
150150

151-
func TestCompareBySeverity(t *testing.T) {
152-
testCompareValues(t, bySeverity(), "Compare By Severity", []compareTestCase{
151+
func Test_bySeverity(t *testing.T) {
152+
testCompareValues(t, bySeverity, "Compare By Severity", []compareTestCase{
153153
{issues[0], issues[1], greater}, // medium vs low
154154
{issues[1], issues[2], less}, // low vs high
155155
{issues[2], issues[3], equal}, // high vs high
@@ -165,24 +165,24 @@ func TestCompareBySeverity(t *testing.T) {
165165
})
166166
}
167167

168-
func TestCompareNested(t *testing.T) {
169-
cmp := byFileName().SetNext(byLine().SetNext(byColumn()))
170-
171-
testCompareValues(t, cmp, "Nested Comparing", []compareTestCase{
172-
{issues[1], issues[0], less}, // file_linux.go vs file_windows.go
173-
{issues[2], issues[1], less}, // file_darwin.go vs file_linux.go
174-
{issues[0], issues[1], greater}, // file_windows.go vs file_linux.go
175-
{issues[1], issues[2], greater}, // file_linux.go vs file_darwin.go
176-
{issues[3], issues[2], less}, // file_darwin.go vs file_darwin.go, 10 vs 12
177-
{issues[0], issues[0], equal}, // file_windows.go vs file_windows.go
178-
{issues[2], issues[3], greater}, // file_darwin.go vs file_darwin.go, 12 vs 10
179-
{issues[1], issues[1], equal}, // file_linux.go vs file_linux.go
180-
{issues[2], issues[2], equal}, // file_darwin.go vs file_darwin.go
181-
{issues[3], issues[3], equal}, // file_darwin.go vs file_darwin.go
182-
})
168+
func Test_mergeComparators(t *testing.T) {
169+
testCompareValues(t, mergeComparators(byFileName, byLine, byColumn), "Nested Comparing",
170+
[]compareTestCase{
171+
{issues[1], issues[0], less}, // file_linux.go vs file_windows.go
172+
{issues[2], issues[1], less}, // file_darwin.go vs file_linux.go
173+
{issues[0], issues[1], greater}, // file_windows.go vs file_linux.go
174+
{issues[1], issues[2], greater}, // file_linux.go vs file_darwin.go
175+
{issues[3], issues[2], less}, // file_darwin.go vs file_darwin.go, 10 vs 12
176+
{issues[0], issues[0], equal}, // file_windows.go vs file_windows.go
177+
{issues[2], issues[3], greater}, // file_darwin.go vs file_darwin.go, 12 vs 10
178+
{issues[1], issues[1], equal}, // file_linux.go vs file_linux.go
179+
{issues[2], issues[2], equal}, // file_darwin.go vs file_darwin.go
180+
{issues[3], issues[3], equal}, // file_darwin.go vs file_darwin.go
181+
},
182+
)
183183
}
184184

185-
func TestNumericCompare(t *testing.T) {
185+
func Test_numericCompare(t *testing.T) {
186186
tests := []struct {
187187
a, b int
188188
expected int
@@ -231,56 +231,6 @@ func TestSorting(t *testing.T) {
231231
assert.Equal(t, []result.Issue{issues[3], issues[2], issues[1], issues[0]}, results)
232232
}
233233

234-
func Test_mergeComparators(t *testing.T) {
235-
testCases := []struct {
236-
desc string
237-
cmps []*comparator
238-
expected string
239-
}{
240-
{
241-
desc: "one",
242-
cmps: []*comparator{byLinter()},
243-
expected: "byLinter",
244-
},
245-
{
246-
desc: "two",
247-
cmps: []*comparator{byLinter(), byFileName()},
248-
expected: "byLinter > byFileName",
249-
},
250-
{
251-
desc: "all",
252-
cmps: []*comparator{bySeverity(), byLinter(), byFileName(), byLine(), byColumn()},
253-
expected: "bySeverity > byLinter > byFileName > byLine > byColumn",
254-
},
255-
{
256-
desc: "nested",
257-
cmps: []*comparator{bySeverity(), byFileName().SetNext(byLine().SetNext(byColumn())), byLinter()},
258-
expected: "bySeverity > byFileName > byLine > byColumn > byLinter",
259-
},
260-
{
261-
desc: "all reverse",
262-
cmps: []*comparator{byColumn(), byLine(), byFileName(), byLinter(), bySeverity()},
263-
expected: "byColumn > byLine > byFileName > byLinter > bySeverity",
264-
},
265-
}
266-
267-
for _, test := range testCases {
268-
t.Run(test.desc, func(t *testing.T) {
269-
t.Parallel()
270-
271-
cmp, err := mergeComparators(test.cmps)
272-
require.NoError(t, err)
273-
274-
assert.Equal(t, test.expected, cmp.String())
275-
})
276-
}
277-
}
278-
279-
func Test_mergeComparators_error(t *testing.T) {
280-
_, err := mergeComparators(nil)
281-
require.EqualError(t, err, "no comparator")
282-
}
283-
284234
func compToString(c int) string {
285235
switch c {
286236
case less:

0 commit comments

Comments
 (0)