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

Collapse output to 20 items #160

Open
wants to merge 8 commits into
base: next
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all 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
5 changes: 5 additions & 0 deletions .changeset/clever-moose-occur.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@steiger/pretty-reporter': minor
---

Collapse the report to 20 diagnostics, add message about hidden diagnostics
3 changes: 2 additions & 1 deletion packages/pretty-reporter/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
"chalk": "^5.3.0",
"figures": "^6.1.0",
"picocolors": "^1.1.1",
"terminal-link": "^3.0.0"
"terminal-link": "^3.0.0",
"vitest": "^3.0.0-beta.2"
}
}
141 changes: 141 additions & 0 deletions packages/pretty-reporter/src/collapse-diagnostics.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
import { describe, expect, it } from 'vitest'
import { trimDiagnosticsToMeetQuota } from './collapse-diagnostics.js'

const defaultLocation = {
path: '/users/user/file',
}

const defaultSeverity = 'error' as 'warn' | 'error'

// dummy rules
Copy link
Member

Choose a reason for hiding this comment

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

suggestion (non-blocking): how about we instead write a function to generate dummy diagnostics with a simple index in the message and then call it in the test setup? This way we can even test with quota = 20

function makeDiagnostic(ruleName: string, index: number): PartialDiagnostic
const diagnosticPerRule = [
  new Array(15).fill(0).map((_, index) => makeDiagnostic('rule-1', index)),
  new Array(10).fill(0).map((_, index) => makeDiagnostic('rule-2', index)),
]

Copy link
Member

Choose a reason for hiding this comment

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

And in the tests that actually drop some diagnostics, we can assert by doing diagnosticPerRule[0].slice(0, 5). This way we also test that it doesn't change the order of diagnostics within one rule


const rule1Message1 = {
message: 'First rule, first message',
ruleName: 'rule-1',
location: defaultLocation,
severity: defaultSeverity,
}

const rule1Message2 = {
message: 'First rule, second message',
ruleName: 'rule-1',
location: defaultLocation,
severity: defaultSeverity,
}

const rule1Message3 = {
message: 'First rule, third message',
ruleName: 'rule-1',
location: defaultLocation,
severity: defaultSeverity,
}

const rule2Message1 = {
message: 'Second rule, first message',
ruleName: 'rule-2',
location: defaultLocation,
severity: defaultSeverity,
}

const rule2Message2 = {
message: 'Second rule, second message',
ruleName: 'rule-2',
location: defaultLocation,
severity: defaultSeverity,
}

const rule3Message1 = {
message: 'Third rule, first message',
ruleName: 'rule-3',
location: defaultLocation,
severity: defaultSeverity,
}

const rule3Message2 = {
message: 'Third rule, second message',
ruleName: 'rule-3',
location: defaultLocation,
severity: defaultSeverity,
}

const rule4Message1 = {
message: 'Forth rule, first message',
ruleName: 'rule-4',
location: defaultLocation,
severity: defaultSeverity,
}

const rule4Message2 = {
message: 'Forth rule, second message',
ruleName: 'rule-4',
location: defaultLocation,
severity: defaultSeverity,
}

describe('trimDiagnosticsToMeetQuota', () => {
it('should return the same diagnostics if they are below the quota', () => {
const diagnosticPerRule = [
[rule1Message1, rule1Message2, rule1Message3],
[rule2Message1, rule2Message2],
]

const result = trimDiagnosticsToMeetQuota(diagnosticPerRule, 10)

expect(result).toEqual(diagnosticPerRule)
})

it('should return no diagnostics if the quota is 0', () => {
const diagnosticPerRule = [
[rule1Message1, rule1Message2, rule1Message3],
[rule2Message1, rule2Message2],
]

const result = trimDiagnosticsToMeetQuota(diagnosticPerRule, 0)

expect(result).toEqual([[], []])
})

it('should spread the quota evenly between rules', () => {
const diagnosticPerRule = [
[rule1Message1, rule1Message2, rule1Message3],
[rule2Message1, rule2Message2],
]

const result = trimDiagnosticsToMeetQuota(diagnosticPerRule, 2)

expect(result).toEqual([[rule1Message1], [rule2Message1]])
})

it('should take first x rule diagnostics if the quota is less than the number of rules', () => {
const diagnosticPerRule = [
[rule1Message1, rule1Message2, rule1Message3],
[rule2Message1, rule2Message2],
[rule3Message1, rule3Message2],
[rule4Message1, rule4Message2],
]

const result = trimDiagnosticsToMeetQuota(diagnosticPerRule, 3)

expect(result).toEqual([[rule1Message1], [rule2Message1], [rule3Message1]])
Copy link
Member

Choose a reason for hiding this comment

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

note: for some reason, this test fails for me locally because result also includes an empty array at the end

})

it('should not distribute the quota to non-existent items', () => {
const diagnostics = [[rule1Message1, rule1Message2, rule1Message3], [rule2Message1, rule2Message2], [rule3Message1]]

expect(trimDiagnosticsToMeetQuota(diagnostics, 6)).toEqual([
[rule1Message1, rule1Message2, rule1Message3],
[rule2Message1, rule2Message2],
[rule3Message1],
])
})

it('should correctly stop distributing quota if the quota is greater than the number of diagnostics', () => {
const diagnostics = [[rule1Message1, rule1Message2, rule1Message3], [rule2Message1, rule2Message2], [rule3Message1]]

expect(trimDiagnosticsToMeetQuota(diagnostics, 10)).toEqual([
[rule1Message1, rule1Message2, rule1Message3],
[rule2Message1, rule2Message2],
[rule3Message1],
])
})
})
51 changes: 51 additions & 0 deletions packages/pretty-reporter/src/collapse-diagnostics.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import { Diagnostic } from '@steiger/types'

const DIAGNOSTIC_QUOTA = 20

function distributeQuota(buckets: Array<number>, quota: number) {
const allItems = buckets.reduce((acc, bucket) => acc + bucket, 0)
const quotaPerBucket = buckets.slice(0).fill(0)
let remainingQuota = quota

for (let i = 0; remainingQuota > 0; i += 1) {
Copy link
Member

Choose a reason for hiding this comment

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

issue: the fact that this is a for loop has made it quite difficult for me to understand the rest of the loop body. A for loop is meant to take a counter variable from A to B, but this one only specifies the A, so it's not a for loop in the traditional sense. Other strange things arise as a consequence, such as assigning i = -1, which is not a valid index, but will be made valid by the mechanics of the for loop

suggestion: let's make this a while loop, I think it more accurately reflects what we're trying to do here:

while (there is quota left) {
  assign one unit of quota
}

const numOfItemsInBucket = buckets[i]
const assignedQuotaForBucket = quotaPerBucket[i]

// If it went beyond the last bucket, start from the first one
if (numOfItemsInBucket === undefined) {
i = -1
continue
}

// If the bucket already has the quota distributed for it or does not contain any items, skip it
if (assignedQuotaForBucket < numOfItemsInBucket && numOfItemsInBucket !== 0) {
quotaPerBucket[i] += 1
remainingQuota -= 1
}

// If it ran out of the items earlier than the quota, break
if (allItems === quota - remainingQuota) {
break
}
Comment on lines +26 to +29
Copy link
Member

Choose a reason for hiding this comment

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

issue: this condition also broke my head for a solid couple of minutes :D

suggestion: instead of checking this on every iteration, let's instead assign remainingQuota = Math.min(quota, allItems), such that there is never more quota than the total number of items

}

return quotaPerBucket
}

function sortDiagnostics(diagnostics: Diagnostic[]): Diagnostic[] {
const severityOrder = { error: 1, warn: 2 }

return diagnostics.slice(0).sort((a, b) => severityOrder[a.severity] - severityOrder[b.severity])
}

// Export for testing purposes to be able to pass a custom quota
Copy link
Member

Choose a reason for hiding this comment

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

issue: I'm not a big fan of using one thing but testing another, it slightly undermines my confidence in the tests

suggestion: how about we make collapseDiagnostics accept the quota as an argument, and move the 20 constant to src/index.ts instead?

export function trimDiagnosticsToMeetQuota(diagnosticsPerRule: Array<Array<Diagnostic>>, quota: number) {
const diagnosticCountPerRule = diagnosticsPerRule.map((diagnostics) => diagnostics.length)
const quotaPerRule = distributeQuota(diagnosticCountPerRule, quota)
const sortedDiagnosticsPerRule = diagnosticsPerRule.map((diagnostics) => sortDiagnostics(diagnostics))

return sortedDiagnosticsPerRule.map((diagnostics, i) => diagnostics.slice(0, quotaPerRule[i]))
}

export const collapseDiagnostics = (diagnosticPerRule: Array<Array<Diagnostic>>) =>
trimDiagnosticsToMeetQuota(diagnosticPerRule, DIAGNOSTIC_QUOTA)
74 changes: 74 additions & 0 deletions packages/pretty-reporter/src/group-diagnostics-by-rule.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
import { Diagnostic } from '@steiger/types'

export function groupDiagnosticsByRule(diagnostics: Diagnostic[]): Diagnostic[][] {
const grouped: Record<string, Diagnostic[]> = {}

diagnostics.forEach((diagnostic) => {
if (!grouped[diagnostic.ruleName]) {
grouped[diagnostic.ruleName] = []
}
grouped[diagnostic.ruleName].push(diagnostic)
})

return Object.values(grouped)
}

if (import.meta.vitest) {
const { describe, it, expect } = import.meta.vitest

describe('groupDiagnosticsByRule', () => {
it('should group diagnostics by ruleName', () => {
const diagnostics: Diagnostic[] = [
{
ruleName: 'rule-1',
severity: 'error',
location: { path: '/users/user/project/src/app/index.ts' },
message: 'First rule, first message',
},
{
ruleName: 'rule-2',
severity: 'warn',
location: { path: '/users/user/project/src/app/index.ts' },
message: 'Second rule, first message',
},
{
ruleName: 'rule-1',
severity: 'warn',
location: { path: '/users/user/project/src/app/index.ts' },
message: 'First rule, second message',
},
]

const result = groupDiagnosticsByRule(diagnostics)

expect(result).toHaveLength(2)
expect(result).toContainEqual([
{
ruleName: 'rule-1',
severity: 'error',
location: { path: '/users/user/project/src/app/index.ts' },
message: 'First rule, first message',
},
{
ruleName: 'rule-1',
severity: 'warn',
location: { path: '/users/user/project/src/app/index.ts' },
message: 'First rule, second message',
},
])
expect(result).toContainEqual([
{
ruleName: 'rule-2',
severity: 'warn',
location: { path: '/users/user/project/src/app/index.ts' },
message: 'Second rule, first message',
},
])
})

it('should return an empty array when no diagnostics are provided', () => {
const result = groupDiagnosticsByRule([])
expect(result).toEqual([])
})
})
}
17 changes: 13 additions & 4 deletions packages/pretty-reporter/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,19 @@ import figures from 'figures'
import type { Diagnostic } from '@steiger/types'

import { formatSingleDiagnostic } from './format-single-diagnostic.js'
import { collapseDiagnostics } from './collapse-diagnostics.js'
import { groupDiagnosticsByRule } from './group-diagnostics-by-rule.js'
import { s } from './pluralization.js'

export function formatPretty(diagnostics: Array<Diagnostic>, cwd: string) {
if (diagnostics.length === 0) {
return pc.green(`${figures.tick} No problems found!`)
}

const collapsedDiagnostics = collapseDiagnostics(groupDiagnosticsByRule(diagnostics)).flat()
const collapsedDiagnosticsCount = collapsedDiagnostics.length
const initialDiagnosticsCount = diagnostics.length

const errors = diagnostics.filter((d) => d.severity === 'error')
const warnings = diagnostics.filter((d) => d.severity === 'warn')

Expand All @@ -22,8 +28,8 @@ export function formatPretty(diagnostics: Array<Diagnostic>, cwd: string) {
.filter(Boolean)
.join(' and ')

const autofixable = diagnostics.filter((d) => (d.fixes?.length ?? 0) > 0)
if (autofixable.length === diagnostics.length) {
const autofixable = collapsedDiagnostics.filter((d) => (d.fixes?.length ?? 0) > 0)
if (autofixable.length === collapsedDiagnostics.length) {
Comment on lines +31 to +32
Copy link
Member

Choose a reason for hiding this comment

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

issue: if I understand correctly, this code will only report how many of the printed 20 diagnostics are fixable. This might be misleading because the autofix engine doesn't know about this collapsing, so it will apply more fixes than what's promised by the output

suggestion: I think we should count and apply fixes from all diagnostics, not just the collapsed ones

footer += ` (all can be fixed automatically with ${pc.bold(pc.green('--fix'))})`
} else if (autofixable.length > 0) {
footer += ` (${autofixable.length} can be fixed automatically with ${pc.bold(pc.green('--fix'))})`
Expand All @@ -33,13 +39,16 @@ export function formatPretty(diagnostics: Array<Diagnostic>, cwd: string) {

return (
'\n' +
diagnostics.map((d) => formatSingleDiagnostic(d, cwd)).join('\n\n') +
collapsedDiagnostics.map((d) => formatSingleDiagnostic(d, cwd)).join('\n\n') +
'\n\n' +
// Due to formatting characters, it won't be exactly the size of the footer, that is okay
pc.gray(figures.line.repeat(footer.length)) +
'\n ' +
footer +
'\n'
'\n ' +
(collapsedDiagnosticsCount < initialDiagnosticsCount
? `${pc.reset(initialDiagnosticsCount - collapsedDiagnosticsCount)} diagnostics are not shown in the report as they exceed the limit allowed by Steiger`
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: yeah, I had an idea to actually tell people how to see all diagnostics instead of this technical "exceed the limit" stuff that Biome prints

Suggested change
? `${pc.reset(initialDiagnosticsCount - collapsedDiagnosticsCount)} diagnostics are not shown in the report as they exceed the limit allowed by Steiger`
? `${pc.reset(initialDiagnosticsCount - collapsedDiagnosticsCount)} diagnostics are not shown, use ${pc.bold(pc.green('--reporter json'))} to see them.`

Copy link
Member

Choose a reason for hiding this comment

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

issue: this will print "1 diagnostics" when only 1 diagnostic was hidden

suggestion: use the s function (imported in this file) to conditionally pluralize the word

: '')
)
}

Expand Down
3 changes: 2 additions & 1 deletion packages/pretty-reporter/tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
"extends": "@steiger/tsconfig/base.json",
"include": ["./src", "./reset.d.ts", "example/index.ts"],
"compilerOptions": {
"tsBuildInfoFile": "node_modules/.cache/tsbuildinfo.json"
"tsBuildInfoFile": "node_modules/.cache/tsbuildinfo.json",
"types": ["vitest/importMeta"]
}
}
7 changes: 7 additions & 0 deletions packages/pretty-reporter/vitest.config.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
import { defineConfig } from 'vitest/config'

export default defineConfig({
test: {
includeSource: ['src/**/*.{js,ts}'],
},
})
Loading
Loading