diff --git a/.github/release.yml b/.github/release.yml index d399c9e..e93eadd 100644 --- a/.github/release.yml +++ b/.github/release.yml @@ -40,4 +40,4 @@ changelog: - title: Other Changes labels: - - "*" + - "*" \ No newline at end of file diff --git a/README.md b/README.md index f47c469..10e0f71 100644 --- a/README.md +++ b/README.md @@ -185,7 +185,7 @@ npm run typecheck npm run check # Scan a directory manually -ast-grep scan --config rules/rust/sgconfig.yml /path/to/code +ast-grep scan --config src/rust/sgconfig.yml /path/to/code ``` ## Contributing diff --git a/package.json b/package.json index 7ba094f..b286a06 100644 --- a/package.json +++ b/package.json @@ -21,7 +21,7 @@ "test:shared-rules": "vitest run", "test:update-snapshots": "sg test --config src/shared/sgconfig.yml --update-all && sg test --config src/rust/sgconfig.yml --update-all && sg test --config src/typescript/sgconfig.yml --update-all", "test:update-shared-snapshots": "sg test --config src/shared/sgconfig.yml --update-all", - "check": "biome check --write --unsafe", + "check": "npm run typecheck && biome check --write --unsafe", "check:biome": "biome check --write --unsafe", "typecheck": "tsc --noEmit", "lint": "biome check", diff --git a/rules/rust/sgconfig.yml b/rules/rust/sgconfig.yml deleted file mode 100644 index e93f2ac..0000000 --- a/rules/rust/sgconfig.yml +++ /dev/null @@ -1,5 +0,0 @@ -ruleDirs: - - rules - -testConfigs: - - testDir: tests diff --git a/rules/typescript/sgconfig.yml b/rules/typescript/sgconfig.yml deleted file mode 100644 index e93f2ac..0000000 --- a/rules/typescript/sgconfig.yml +++ /dev/null @@ -1,5 +0,0 @@ -ruleDirs: - - rules - -testConfigs: - - testDir: tests diff --git a/scripts/post-pr-comment.test.ts b/scripts/post-pr-comment.test.ts index 860ad2a..b9c2e48 100644 --- a/scripts/post-pr-comment.test.ts +++ b/scripts/post-pr-comment.test.ts @@ -1,67 +1,10 @@ import { describe, expect, it } from 'vitest' -import { - COMMENT_SIGNATURE, - countBySeverity, - type LintIssue, - MAX_ISSUES_IN_COMMENT, -} from './shared.ts' +import { generateCommentBody } from './post-pr-comment.ts' +import { COMMENT_SIGNATURE, type LintIssue } from './shared.ts' -/** - * Test the comment generation logic. - * Since generateComment in post-pr-comment.ts uses module-scoped variables, - * we recreate the logic here for testability. - */ -function generateComment(issues: LintIssue[], totalIssues: number): string { - const signature = COMMENT_SIGNATURE - - if (totalIssues === 0) { - return `${signature} -## [PASS] Tempo Lint Results - -No lint issues found! Great job!` - } - - const counts = countBySeverity(issues) - - let body = `${signature} -## Tempo Lint Results - -Found **${totalIssues}** issue(s) in this PR. - -| Severity | Count | -|----------|-------| -| Errors | ${counts.error} | -| Warnings | ${counts.warning} | -| Hints | ${counts.hint} | - -
-View Issues - -\`\`\` -` - - const displayIssues = issues.slice(0, MAX_ISSUES_IN_COMMENT) - for (const item of displayIssues) { - body += `${item.file}:${item.line} [${item.severity}] ${item.ruleId}: ${item.message}\n` - } - - if (issues.length > MAX_ISSUES_IN_COMMENT) { - body += `\n... and ${issues.length - MAX_ISSUES_IN_COMMENT} more issues\n` - } - - body += `\`\`\` - -
- ---- -*Posted by https://github.com/tempoxyz/lints*` - - return body -} - -describe('generateComment', () => { +describe('generateCommentBody', () => { it('should generate success comment when no issues', () => { - const comment = generateComment([], 0) + const comment = generateCommentBody([], 0) expect(comment).toContain(COMMENT_SIGNATURE) expect(comment).toContain('[PASS] Tempo Lint Results') @@ -88,17 +31,18 @@ describe('generateComment', () => { }, ] - const comment = generateComment(issues, 2) + const comment = generateCommentBody(issues, 2) expect(comment).toContain(COMMENT_SIGNATURE) expect(comment).toContain('## Tempo Lint Results') - expect(comment).toContain('Found **2** issue(s)') + expect(comment).toContain('### Summary') + expect(comment).toContain('Found **2** issue(s) across **2** file(s)') expect(comment).toContain('| Errors | 1 |') expect(comment).toContain('| Warnings | 1 |') expect(comment).toContain('| Hints | 0 |') }) - it('should include issue details in code block', () => { + it('should include issue details grouped by rule', () => { const issues: LintIssue[] = [ { ruleId: 'no-dbg-macro', @@ -110,35 +54,46 @@ describe('generateComment', () => { }, ] - const comment = generateComment(issues, 1) + const comment = generateCommentBody(issues, 1) - expect(comment).toContain('crates/core/src/lib.rs:42 [error] no-dbg-macro: Remove dbg! macro') - expect(comment).toContain('```') + expect(comment).toContain('### Issues by Rule Type') + expect(comment).toContain('no-dbg-macro') + expect(comment).toContain('`crates/core/src/lib.rs:42` - Remove dbg! macro') expect(comment).toContain('
') - expect(comment).toContain('View Issues') + expect(comment).toContain('### Issues by File') }) - it('should truncate issues when exceeding MAX_ISSUES_IN_COMMENT', () => { - // Create more issues than MAX_ISSUES_IN_COMMENT (which is 25) - const issues: LintIssue[] = Array.from({ length: 30 }, (_, i) => ({ - ruleId: `rule-${i}`, - severity: 'warning', - message: `Issue ${i}`, - file: `file${i}.ts`, - line: i + 1, - column: 1, - })) - - const comment = generateComment(issues, 30) - - // Should only show first 25 issues - expect(comment).toContain('rule-0') - expect(comment).toContain('rule-24') - expect(comment).not.toContain('rule-25') - expect(comment).not.toContain('rule-29') - - // Should show truncation message - expect(comment).toContain('... and 5 more issues') + it('should group and truncate issues by rule', () => { + // Create 15 issues with the same rule and 5 with another + const issues: LintIssue[] = [ + ...Array.from({ length: 15 }, (_, i) => ({ + ruleId: 'common-rule', + severity: 'warning', + message: `Issue ${i}`, + file: `file${i}.ts`, + line: i + 1, + column: 1, + })), + ...Array.from({ length: 5 }, (_, i) => ({ + ruleId: 'rare-rule', + severity: 'error', + message: `Error ${i}`, + file: `error${i}.ts`, + line: i + 1, + column: 1, + })), + ] + + const comment = generateCommentBody(issues, 20) + + // Should show rule groupings + expect(comment).toContain('common-rule') + expect(comment).toContain('(15 occurrences)') + expect(comment).toContain('rare-rule') + expect(comment).toContain('(5 occurrences)') + + // Should truncate common-rule to 10 issues + expect(comment).toContain('*... and 5 more*') }) it('should include footer with link', () => { @@ -153,7 +108,7 @@ describe('generateComment', () => { }, ] - const comment = generateComment(issues, 1) + const comment = generateCommentBody(issues, 1) expect(comment).toContain('Posted by https://github.com/tempoxyz/lints') }) @@ -168,7 +123,7 @@ describe('generateComment', () => { { ruleId: 'r6', severity: 'hint', message: 'Hint 1', file: 'f6.ts', line: 6, column: 1 }, ] - const comment = generateComment(issues, 6) + const comment = generateCommentBody(issues, 6) expect(comment).toContain('| Errors | 3 |') expect(comment).toContain('| Warnings | 2 |') @@ -187,7 +142,7 @@ describe('generateComment', () => { }, ] - const comment = generateComment(issues, 1) + const comment = generateCommentBody(issues, 1) expect(comment).toContain('') expect(comment).toContain('&') @@ -201,9 +156,3 @@ describe('COMMENT_SIGNATURE', () => { expect(COMMENT_SIGNATURE).toBe('') }) }) - -describe('MAX_ISSUES_IN_COMMENT', () => { - it('should be a reasonable limit', () => { - expect(MAX_ISSUES_IN_COMMENT).toBe(25) - }) -}) diff --git a/scripts/post-pr-comment.ts b/scripts/post-pr-comment.ts index 926d14e..ce1e18e 100644 --- a/scripts/post-pr-comment.ts +++ b/scripts/post-pr-comment.ts @@ -5,39 +5,19 @@ import { COMMENT_SIGNATURE, countBySeverity, getValidRuleIds, + groupByFile, + groupByRule, isValidLanguage, type Language, type LintIssue, - MAX_ISSUES_IN_COMMENT, + MAX_FILES_TO_DISPLAY, + MAX_ISSUES_PER_FILE, + MAX_ISSUES_PER_RULE, parseLintIssues, + pluralize, warn, } from './shared.ts' -const [, , outputFile, totalIssuesStr, repo, prNumber, languageArg] = process.argv -const totalIssues = Number.parseInt(totalIssuesStr ?? '0', 10) || 0 -const githubToken = process.env.GITHUB_TOKEN - -if (!githubToken) { - warn('GITHUB_TOKEN environment variable is required') - process.exit(1) -} - -if (!repo || !prNumber) { - warn('Repository and PR number are required') - console.error( - 'Usage: tsx post-pr-comment.ts ', - ) - process.exit(1) -} - -if (!languageArg || !isValidLanguage(languageArg)) { - warn(`Invalid language: ${languageArg}. Must be one of: rust, typescript, all`) - process.exit(1) -} - -// Type assertion safe after validation -const language: Language = languageArg as Language - interface GitHubComment { id?: number body?: string @@ -62,6 +42,7 @@ async function githubRequest( method: string, path: string, body: object | null = null, + githubToken: string, ): Promise<{ status: number; data: T }> { const url = `https://api.github.com${path}` @@ -93,10 +74,17 @@ async function githubRequest( return { status: response.status, data } } -function generateComment(issues: LintIssue[]): string { +/** + * Generates a PR comment body from lint issues. + * Exported for testing purposes. + * @param issues The lint issues to format + * @param totalIssuesCount The total number of issues (may differ from issues.length if filtered) + * @returns Formatted markdown comment body + */ +export function generateCommentBody(issues: LintIssue[], totalIssuesCount: number): string { const signature = COMMENT_SIGNATURE - if (totalIssues === 0) { + if (totalIssuesCount === 0) { return `${signature} ## [PASS] Tempo Lint Results @@ -104,11 +92,14 @@ No lint issues found! Great job!` } const counts = countBySeverity(issues) + const fileGroups = groupByFile(issues) + const ruleGroups = groupByRule(issues) let body = `${signature} ## Tempo Lint Results -Found **${totalIssues}** issue(s) in this PR. +### Summary +Found **${totalIssuesCount}** issue(s) across **${Object.keys(fileGroups).length}** file(s) | Severity | Count | |----------|-------| @@ -116,35 +107,73 @@ Found **${totalIssues}** issue(s) in this PR. | Warnings | ${counts.warning} | | Hints | ${counts.hint} | -
-View Issues +--- -\`\`\` +### Issues by Rule Type ` - const displayIssues = issues.slice(0, MAX_ISSUES_IN_COMMENT) - for (const item of displayIssues) { - body += `${item.file}:${item.line} [${item.severity}] ${item.ruleId}: ${item.message}\n` - } + // Sort rules by frequency (most common first) + const sortedRules = Object.entries(ruleGroups).sort(([, a], [, b]) => b.length - a.length) - if (issues.length > MAX_ISSUES_IN_COMMENT) { - body += `\n... and ${issues.length - MAX_ISSUES_IN_COMMENT} more issues\n` + for (const [ruleId, ruleIssues] of sortedRules) { + body += `\n
\n${ruleId} (${ruleIssues.length} ${pluralize(ruleIssues.length, 'occurrence')})\n\n` + + // Show up to MAX_ISSUES_PER_RULE issues per rule + const displayCount = Math.min(ruleIssues.length, MAX_ISSUES_PER_RULE) + for (let i = 0; i < displayCount; i++) { + const issue = ruleIssues[i] + if (!issue) continue + body += `- \`${issue.file}:${issue.line}\` - ${issue.message}\n` + } + + if (ruleIssues.length > MAX_ISSUES_PER_RULE) { + body += `\n*... and ${ruleIssues.length - MAX_ISSUES_PER_RULE} more*\n` + } + + body += `\n
\n` } - body += `\`\`\` + // Add "by file" view + body += `\n### Issues by File\n\n
\nView grouped by file\n\n` -
+ const sortedFiles = Object.entries(fileGroups).sort(([, a], [, b]) => b.length - a.length) + const displayFiles = sortedFiles.slice(0, MAX_FILES_TO_DISPLAY) ---- -*Posted by https://github.com/tempoxyz/lints*` + for (const [file, fileIssues] of displayFiles) { + body += `\n**${file}** (${fileIssues.length} ${pluralize(fileIssues.length, 'issue')})\n` + + // Show up to MAX_ISSUES_PER_FILE issues per file + const displayCount = Math.min(fileIssues.length, MAX_ISSUES_PER_FILE) + for (let i = 0; i < displayCount; i++) { + const issue = fileIssues[i] + if (!issue) continue + body += `- Line ${issue.line}: [${issue.severity}] ${issue.ruleId}\n` + } + + if (fileIssues.length > MAX_ISSUES_PER_FILE) { + body += `- *... and ${fileIssues.length - MAX_ISSUES_PER_FILE} more*\n` + } + } + + if (sortedFiles.length > MAX_FILES_TO_DISPLAY) { + body += `\n*Showing ${MAX_FILES_TO_DISPLAY} of ${sortedFiles.length} files*\n` + } + + body += `\n
\n\n---\n*Posted by https://github.com/tempoxyz/lints*` return body } -async function findExistingComment(): Promise { +async function findExistingComment( + repo: string, + prNumber: string, + githubToken: string, +): Promise { const { data } = await githubRequest( 'GET', `/repos/${repo}/issues/${prNumber}/comments`, + null, + githubToken, ) if (!Array.isArray(data)) return null @@ -158,7 +187,14 @@ async function findExistingComment(): Promise { return null } -async function main(): Promise { +async function main( + outputFile: string, + totalIssues: number, + repo: string, + prNumber: string, + language: Language, + githubToken: string, +): Promise { let issues: LintIssue[] = [] if (outputFile && fs.existsSync(outputFile)) { try { @@ -175,8 +211,8 @@ async function main(): Promise { } } - const commentBody = generateComment(issues) - const existingCommentId = await findExistingComment() + const commentBody = generateCommentBody(issues, totalIssues) + const existingCommentId = await findExistingComment(repo, prNumber, githubToken) try { if (existingCommentId) { @@ -185,12 +221,16 @@ async function main(): Promise { 'PATCH', `/repos/${repo}/issues/comments/${existingCommentId}`, { body: commentBody }, + githubToken, ) } else { console.log('Creating new comment...') - await githubRequest('POST', `/repos/${repo}/issues/${prNumber}/comments`, { - body: commentBody, - }) + await githubRequest( + 'POST', + `/repos/${repo}/issues/${prNumber}/comments`, + { body: commentBody }, + githubToken, + ) } console.log('PR comment posted successfully!') @@ -200,7 +240,35 @@ async function main(): Promise { } } -main().catch((err: Error) => { - warn(err.message) - process.exit(1) -}) +// Only run if this file is executed directly +if (import.meta.url === `file://${process.argv[1]}`) { + const [, , outputFile, totalIssuesStr, repo, prNumber, languageArg] = process.argv + const totalIssues = Number.parseInt(totalIssuesStr ?? '0', 10) || 0 + const githubToken = process.env.GITHUB_TOKEN + + if (!githubToken) { + warn('GITHUB_TOKEN environment variable is required') + process.exit(1) + } + + if (!repo || !prNumber) { + warn('Repository and PR number are required') + console.error( + 'Usage: tsx post-pr-comment.ts ', + ) + process.exit(1) + } + + if (!languageArg || !isValidLanguage(languageArg)) { + warn(`Invalid language: ${languageArg}. Must be one of: rust, typescript, all`) + process.exit(1) + } + + // Type assertions safe after validation + const language: Language = languageArg as Language + + main(outputFile ?? '', totalIssues, repo, prNumber, language, githubToken).catch((err: Error) => { + warn(err.message) + process.exit(1) + }) +} diff --git a/scripts/shared.ts b/scripts/shared.ts index 86d6c8f..d1db652 100644 --- a/scripts/shared.ts +++ b/scripts/shared.ts @@ -21,7 +21,9 @@ const SEVERITY = { HINT: 'hint', } as const -export const MAX_ISSUES_IN_COMMENT = 25 +export const MAX_ISSUES_PER_RULE = 10 +export const MAX_ISSUES_PER_FILE = 5 +export const MAX_FILES_TO_DISPLAY = 10 export const PACKAGE_ROOT = path.resolve(path.dirname(new URL(import.meta.url).pathname), '..') @@ -266,6 +268,43 @@ export function countBySeverity(issues: LintIssue[]): SeverityCounts { return counts } +/** + * Generic groupBy function that groups items by a key selector. + * @param items Array of items to group + * @param keySelector Function that extracts the key from each item + * @returns Record with keys and arrays of grouped items + */ +export function groupBy(items: T[], keySelector: (item: T) => string): Record { + return items.reduce( + (acc, item) => { + const key = keySelector(item) + if (!acc[key]) acc[key] = [] + acc[key].push(item) + return acc + }, + {} as Record, + ) +} + +export function groupByFile(issues: LintIssue[]): Record { + return groupBy(issues, (issue) => issue.file) +} + +export function groupByRule(issues: LintIssue[]): Record { + return groupBy(issues, (issue) => issue.ruleId) +} + +/** + * Returns a pluralized string based on the count. + * @param count The count to check + * @param singular The singular form of the word + * @param plural The plural form (defaults to singular + 's') + * @returns The word with appropriate form + */ +export function pluralize(count: number, singular: string, plural?: string): string { + return count === 1 ? singular : (plural ?? `${singular}s`) +} + export function warn(message: string): void { console.error(`[tempo-lints] warning: ${message}`) } diff --git a/shared/tests/rust/no-emojis-test.yml b/shared/tests/rust/no-emojis-test.yml deleted file mode 100644 index eef44b4..0000000 --- a/shared/tests/rust/no-emojis-test.yml +++ /dev/null @@ -1,16 +0,0 @@ -id: no-emojis -valid: - - | - let msg = "Hello world"; - - | - let msg = "Success!"; - - | - let msg = "Error: something went wrong"; - -invalid: - - | - let msg = "Hello 🌍"; - - | - let msg = "Success 🎉"; - - | - let emoji = "🚀"; diff --git a/shared/tests/rust/no-leading-whitespace-strings-test.yml b/shared/tests/rust/no-leading-whitespace-strings-test.yml deleted file mode 100644 index 908a029..0000000 --- a/shared/tests/rust/no-leading-whitespace-strings-test.yml +++ /dev/null @@ -1,16 +0,0 @@ -id: no-leading-whitespace-strings -valid: - - | - let msg = "hello"; - - | - let msg = " indented"; - - | - let msg = "\t\ttabbed"; - - | - let fmt = "{} items"; - -invalid: - - | - let msg = " hello"; - - | - let msg = " world"; diff --git a/shared/tests/typescript/no-emojis-test.yml b/shared/tests/typescript/no-emojis-test.yml deleted file mode 100644 index fc9a677..0000000 --- a/shared/tests/typescript/no-emojis-test.yml +++ /dev/null @@ -1,16 +0,0 @@ -id: no-emojis -valid: - - | - const msg = "Hello world"; - - | - const msg = "Success!"; - - | - const msg = "Error: something went wrong"; - -invalid: - - | - const msg = "Hello 🌍"; - - | - const msg = "Success 🎉"; - - | - const emoji = "🚀"; diff --git a/shared/tests/typescript/no-leading-whitespace-strings-test.yml b/shared/tests/typescript/no-leading-whitespace-strings-test.yml deleted file mode 100644 index 8e8226f..0000000 --- a/shared/tests/typescript/no-leading-whitespace-strings-test.yml +++ /dev/null @@ -1,16 +0,0 @@ -id: no-leading-whitespace-strings -valid: - - | - const msg = "hello"; - - | - const msg = " indented"; - - | - const msg = "\t\ttabbed"; - - | - const fmt = "{} items"; - -invalid: - - | - const msg = " hello"; - - | - const msg = " world"; diff --git a/test-fixtures/rust/mixed.rs b/test-fixtures/rust/mixed.rs deleted file mode 100644 index 1d73e12..0000000 --- a/test-fixtures/rust/mixed.rs +++ /dev/null @@ -1,14 +0,0 @@ -// This file contains multiple types of violations - -pub fn process_data(input: Option) -> String { - let data = input.unwrap(); // no-unwrap-in-lib violation - - dbg!(&data); // no-dbg-macro violation - - data.to_uppercase() -} - -pub fn debug_value(x: i32) { - dbg!(x); // Another dbg! violation - println!("Value: {}", x); -} diff --git a/test-fixtures/rust/valid.rs b/test-fixtures/rust/valid.rs deleted file mode 100644 index 6dfc0c6..0000000 --- a/test-fixtures/rust/valid.rs +++ /dev/null @@ -1,19 +0,0 @@ -// This file should have no lint violations - -fn add(a: i32, b: i32) -> i32 { - a + b -} - -fn multiply(a: i32, b: i32) -> i32 { - a * b -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn test_add() { - assert_eq!(add(2, 3), 5); - } -} diff --git a/test-fixtures/rust/with-dbg.rs b/test-fixtures/rust/with-dbg.rs index 4c33e09..d16c251 100644 --- a/test-fixtures/rust/with-dbg.rs +++ b/test-fixtures/rust/with-dbg.rs @@ -1,12 +1,4 @@ -// This file contains dbg!() macro which should be flagged - -fn calculate(x: i32, y: i32) -> i32 { - let result = x + y; - dbg!(result); // This should be caught by no-dbg-macro rule - result -} - fn main() { - let value = 42; - dbg!(value); // Another dbg! to catch + let x = 42; + dbg!(x); } diff --git a/test-fixtures/rust/with-unwrap.rs b/test-fixtures/rust/with-unwrap.rs index ddc4216..e2e13be 100644 --- a/test-fixtures/rust/with-unwrap.rs +++ b/test-fixtures/rust/with-unwrap.rs @@ -1,14 +1,3 @@ -// This file contains .unwrap() calls which should be flagged in library code - -pub fn parse_number(s: &str) -> i32 { - s.parse::().unwrap() // Should be caught by no-unwrap-in-lib -} - -pub fn get_first(vec: Vec) -> T { - vec.into_iter().next().unwrap() // Another unwrap to catch -} - -fn main() { - let result = parse_number("42"); - println!("{}", result); +pub fn parse_config(s: &str) -> i32 { + s.parse().unwrap() }