diff --git a/README.md b/README.md index ee48181..4ba010f 100644 --- a/README.md +++ b/README.md @@ -82,7 +82,7 @@ To pass additional arguments to `dprint check`, pass them to the `args` input. ### Annotations -When a file isn't formatted, the action emits an error annotation for it, which GitHub shows on the pull request's changed files and in the check summary. The annotation points at the first change and includes the diff. This requires dprint 0.57 or later and `node` on the path (always the case on GitHub-hosted runners); otherwise the action only outputs the diffs to the log. Note that GitHub shows at most 10 error annotations per step, so the log is the complete list. +When a file isn't formatted, the action emits an error annotation for it, which GitHub shows on the pull request's changed files and in the check summary. The annotation points at the first change and lists the changed lines of the diff. This requires dprint 0.57 or later and `node` on the path (always the case on GitHub-hosted runners); otherwise the action only outputs the diffs to the log. Note that GitHub shows at most 10 error annotations per step, so the log is the complete list. To disable annotations: diff --git a/scripts/annotate.mjs b/scripts/annotate.mjs index 60ab95d..8379f26 100644 --- a/scripts/annotate.mjs +++ b/scripts/annotate.mjs @@ -5,7 +5,9 @@ import fs from "node:fs"; import path from "node:path"; -const MAX_ANNOTATION_MESSAGE_LENGTH = 4000; +// the full diff is in the log, so keep the annotation short +const MAX_ANNOTATION_DIFF_LINES = 10; +const MAX_ANNOTATION_DIFF_LENGTH = 4000; const LINE_ENDINGS_MESSAGE = "Text differed by line endings."; const WINDOWS_LINE_ENDINGS_HINT = "Git on Windows runners checks out files with CRLF line endings, so consider only running this action on Linux: https://github.com/dprint/check#windows-line-endings"; @@ -32,13 +34,14 @@ const entries = fs.readFileSync(jsonlPath, "utf8") for (const entry of entries) { const relativePath = toRelativePath(entry.file, workspace); + const changes = entry.diff == null ? undefined : parseChanges(entry.diff); // a diff that only changes line endings is every line of the file, so // summarize it like dprint's default output does - const lineEndings = entry.diff == null ? undefined : getLineEndingsOnlyChange(entry.diff); + const lineEndings = changes == null ? undefined : getLineEndingsOnlyChange(changes); console.log(`from ${relativePath}:`); console.log(describeDiff(entry.diff, lineEndings)); console.log("--"); - console.log(annotation(relativePath, entry.diff, lineEndings)); + console.log(annotation(relativePath, changes, lineEndings)); } if (entries.length > 0) { @@ -55,10 +58,10 @@ function describeDiff(diff, lineEndings) { } /** Builds the `::error` workflow command for a file that isn't formatted. */ -function annotation(relativePath, diff, lineEndings) { +function annotation(relativePath, changes, lineEndings) { // the whole file differs when only the line endings do, so point at the top // of it rather than highlighting every line - const range = lineEndings != null ? { line: 1, endLine: 1 } : firstHunkRange(diff); + const range = changes == null || lineEndings != null ? { line: 1, endLine: 1 } : changeRange(changes[0]); const properties = { file: relativePath, line: range.line, title: "dprint" }; if (range.endLine !== range.line) { properties.endLine = range.endLine; @@ -72,66 +75,159 @@ function annotation(relativePath, diff, lineEndings) { if (lineEndings.original === "crlf" && process.env.RUNNER_OS === "Windows") { message += " " + WINDOWS_LINE_ENDINGS_HINT; } - } else if (diff != null) { - message += "\n" + truncate(makePrintable(stripDiffHeader(diff)), MAX_ANNOTATION_MESSAGE_LENGTH); + } else if (changes != null && changes.length > 0) { + // the annotation is shown beside the file, so the surrounding lines are + // already visible and only the changed lines are worth repeating + message += "\n\n" + makePrintable(formatChanges(changes)); } return `::error ${propertiesText}::${escapeMessage(message)}`; } /** - * Gets the range of lines in the original file covered by the first hunk of - * a unified diff, falling back to the first line when there's no hunk. + * Gets the range of lines in the original file that a change covers. An + * insertion doesn't cover any, so it points at the line it comes after. */ -function firstHunkRange(diff) { - const match = diff == null ? null : /^@@ -(\d+)(?:,(\d+))? \+\d+(?:,\d+)? @@/m.exec(diff); - if (match == null) { +function changeRange(change) { + if (change == null) { return { line: 1, endLine: 1 }; } - // an empty original file has a hunk starting at line 0 - const line = Math.max(Number(match[1]), 1); - // a count of zero means lines are only inserted after this line - const count = Math.max(match[2] == null ? 1 : Number(match[2]), 1); - return { line, endLine: line + count - 1 }; + if (change.oldCount === 0) { + const line = Math.max(change.oldStart - 1, 1); + return { line, endLine: line }; + } + return { line: change.oldStart, endLine: change.oldStart + change.oldCount - 1 }; } /** - * Gets the line ending the original file had when a unified diff only changes - * line endings, which is found by rebuilding both sides of the diff and - * comparing them without carriage returns. Returns `undefined` otherwise. + * Formats the changes as unified diff hunks without context lines, like + * `diff -U0` does, stopping once the annotation would get too long. */ -function getLineEndingsOnlyChange(diff) { - const oldLines = []; - const newLines = []; - let originalHasCarriageReturn = false; - // the sides the previous line was added to, so a "no newline at end of - // file" marker can be applied to it - let previousSides = []; +function formatChanges(changes) { + const lines = []; + let truncated = false; + for (const change of changes) { + const changeLines = formatChange(change); + if (lines.length + changeLines.length > MAX_ANNOTATION_DIFF_LINES) { + truncated = true; + // stop at a change boundary so a removal isn't shown without its + // replacement, unless the first change is too long on its own + if (lines.length === 0) { + lines.push(...changeLines.slice(0, MAX_ANNOTATION_DIFF_LINES)); + } + break; + } + lines.push(...changeLines); + } + let text = lines.join("\n"); + if (text.length > MAX_ANNOTATION_DIFF_LENGTH) { + truncated = true; + text = text.slice(0, MAX_ANNOTATION_DIFF_LENGTH); + } + return truncated ? `${text}\n(truncated, see the log for the full diff)` : text; +} + +function formatChange(change) { + const header = `@@ -${hunkRange(change.oldStart, change.oldCount)} +${ + hunkRange(change.newStart, change.newCount) + } @@`; + return [header, ...change.lines]; +} + +function hunkRange(start, count) { + if (count === 1) { + return `${start}`; + } + // an empty range is written as the line it comes after + return `${count === 0 ? start - 1 : start},${count}`; +} + +/** + * Splits a unified diff into its changes, which are the consecutive runs of + * removed and added lines, along with where they start on each side. + */ +function parseChanges(diff) { + const changes = []; + let current; let inHunk = false; + let oldLine = 0; + let newLine = 0; for (const line of diff.split("\n")) { - if (line.startsWith("@@")) { + const hunk = /^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@/.exec(line); + if (hunk != null) { inHunk = true; + current = undefined; + oldLine = hunkStartLine(hunk[1], hunk[2]); + newLine = hunkStartLine(hunk[3], hunk[4]); continue; } if (!inHunk) { continue; } - if (line.startsWith("\\")) { - for (const side of previousSides) { - side[side.length - 1] += ""; + const sign = line[0]; + if (sign === "-" || sign === "+") { + if (current == null) { + current = { oldStart: oldLine, oldCount: 0, newStart: newLine, newCount: 0, lines: [] }; + changes.push(current); } - continue; + current.lines.push(line); + if (sign === "-") { + current.oldCount++; + oldLine++; + } else { + current.newCount++; + newLine++; + } + } else if (sign === "\\") { + // a "no newline at end of file" marker belongs to the line before it + current?.lines.push(line); + } else { + // a context line ends the current change + current = undefined; + oldLine++; + newLine++; } - const sign = line[0]; - previousSides = sign === "-" ? [oldLines] : sign === "+" ? [newLines] : sign === " " ? [oldLines, newLines] : []; - if (sign === "-" && line.endsWith("\r")) { - originalHasCarriageReturn = true; + } + return changes; +} + +/** Gets the first line of a hunk, where an empty range is written as the line it comes after. */ +function hunkStartLine(start, count) { + const line = Number(start); + return count === "0" ? line + 1 : line; +} + +/** + * Gets the line ending the original file had when a diff only changes line + * endings, which is found by comparing the removed and added lines of each + * change without carriage returns. Returns `undefined` otherwise. + */ +function getLineEndingsOnlyChange(changes) { + let originalHasCarriageReturn = false; + for (const change of changes) { + const oldLines = []; + const newLines = []; + // the side the previous line was added to, so a "no newline at end of + // file" marker can be applied to it + let previousSide; + for (const line of change.lines) { + if (line.startsWith("\\")) { + if (previousSide != null) { + previousSide[previousSide.length - 1] += ""; + } + continue; + } + previousSide = line[0] === "-" ? oldLines : newLines; + if (previousSide === oldLines && line.endsWith("\r")) { + originalHasCarriageReturn = true; + } + previousSide.push(line.slice(1).replace(/\r$/, "")); } - for (const side of previousSides) { - side.push(line.slice(1).replace(/\r$/, "")); + const isOnlyLineEndings = oldLines.length === newLines.length && oldLines.every((line, i) => line === newLines[i]); + if (!isOnlyLineEndings) { + return undefined; } } - const isOnlyLineEndings = oldLines.length === newLines.length && oldLines.every((line, i) => line === newLines[i]); - return isOnlyLineEndings ? { original: originalHasCarriageReturn ? "crlf" : "lf" } : undefined; + return { original: originalHasCarriageReturn ? "crlf" : "lf" }; } function toRelativePath(filePath, workspace) { @@ -140,20 +236,11 @@ function toRelativePath(filePath, workspace) { return (isOutside ? filePath : relativePath).replaceAll("\\", "/"); } -/** Removes the `--- original` and `+++ formatted` lines. */ -function stripDiffHeader(diff) { - return diff.split("\n").filter((line) => !/^(---|\+\+\+) /.test(line)).join("\n"); -} - /** Makes carriage returns visible so a line ending difference is readable. */ function makePrintable(text) { return text.replaceAll("\r", "\\r").trimEnd(); } -function truncate(text, maxLength) { - return text.length <= maxLength ? text : `${text.slice(0, maxLength)}\n(truncated)`; -} - function escapeProperty(value) { return value .replaceAll("%", "%25") diff --git a/scripts/annotate_test.ts b/scripts/annotate_test.ts index 0a40f74..badb36d 100644 --- a/scripts/annotate_test.ts +++ b/scripts/annotate_test.ts @@ -17,7 +17,7 @@ async function runAnnotate(workspace: string, entries: Record[] }; } -Deno.test("annotates each file at the first hunk and prints the diffs", async () => { +Deno.test("annotates each file at the first change and prints the diffs", async () => { const workspace = await Deno.makeTempDir(); try { const result = await runAnnotate(workspace, [{ @@ -41,7 +41,7 @@ Deno.test("annotates each file at the first hunk and prints the diffs", async () "+- a", "+- b", "--", - "::error file=src/bad.md,line=3,title=dprint,endLine=6::File is not formatted. Run `dprint fmt` to fix.%0A@@ -3,4 +3,2 @@%0A # T%0A-* a%0A-* b%0A+- a%0A+- b", + "::error file=src/bad.md,line=4,title=dprint,endLine=5::File is not formatted. Run `dprint fmt` to fix.%0A%0A@@ -4,2 +4,2 @@%0A-* a%0A-* b%0A+- a%0A+- b", "from bad.json:", "--- original", "+++ formatted", @@ -49,7 +49,7 @@ Deno.test("annotates each file at the first hunk and prints the diffs", async () "-{\"a\":1}", "+{ \"a\": 1 }", "--", - "::error file=bad.json,line=1,title=dprint::File is not formatted. Run `dprint fmt` to fix.%0A@@ -1 +1 @@%0A-{\"a\":1}%0A+{ \"a\": 1 }", + "::error file=bad.json,line=1,title=dprint::File is not formatted. Run `dprint fmt` to fix.%0A%0A@@ -1 +1 @@%0A-{\"a\":1}%0A+{ \"a\": 1 }", "Found 2 not formatted files. Run dprint fmt to fix.", "", ]); @@ -58,6 +58,68 @@ Deno.test("annotates each file at the first hunk and prints the diffs", async () } }); +Deno.test("annotates only the changed lines since the surrounding lines are already visible", async () => { + const workspace = await Deno.makeTempDir(); + try { + const result = await runAnnotate(workspace, [{ + file: `${workspace}/context.md`, + diff: "--- original\n+++ formatted\n@@ -8,7 +8,7 @@\n \n a\n \n-## Title\n+## Title\n \n b\n", + }, { + // a hunk can have several changes separated by context lines + file: `${workspace}/two-changes.md`, + diff: "--- original\n+++ formatted\n@@ -1,6 +1,6 @@\n a\n-b \n+b\n c\n d\n e\n-f \n+f\n", + }, { + // an insertion doesn't cover any original lines, so it points at the line before it + file: `${workspace}/insertion.md`, + diff: "--- original\n+++ formatted\n@@ -1,2 +1,3 @@\n a\n+b\n c\n", + }]); + assertEquals(result.stderr, ""); + assertEquals(result.code, 0); + assertStringIncludes( + result.stdout, + "::error file=context.md,line=11,title=dprint::File is not formatted. Run `dprint fmt` to fix.%0A%0A@@ -11 +11 @@%0A-## Title%0A+## Title\n", + ); + assertStringIncludes( + result.stdout, + "::error file=two-changes.md,line=2,title=dprint::File is not formatted. Run `dprint fmt` to fix.%0A%0A@@ -2 +2 @@%0A-b %0A+b%0A@@ -6 +6 @@%0A-f %0A+f\n", + ); + assertStringIncludes( + result.stdout, + "::error file=insertion.md,line=1,title=dprint::File is not formatted. Run `dprint fmt` to fix.%0A%0A@@ -1,0 +2 @@%0A+b\n", + ); + } finally { + await Deno.remove(workspace, { recursive: true }); + } +}); + +Deno.test("truncates a long diff since the full diff is in the log", async () => { + const workspace = await Deno.makeTempDir(); + try { + const result = await runAnnotate(workspace, [{ + // stops at a change boundary so a removal isn't shown without its replacement + file: `${workspace}/two-changes.md`, + diff: + "--- original\n+++ formatted\n@@ -1,12 +1,12 @@\n-a \n-b \n-c \n+a\n+b\n+c\n d\n e\n f\n-g \n-h \n-i \n+g\n+h\n+i\n", + }, { + // a change that's too long on its own is cut short + file: `${workspace}/one-change.md`, + diff: "--- original\n+++ formatted\n@@ -1,6 +1,6 @@\n-a \n-b \n-c \n-d \n-e \n-f \n+a\n+b\n+c\n+d\n+e\n+f\n", + }]); + assertEquals(result.stderr, ""); + assertEquals(result.code, 0); + assertStringIncludes( + result.stdout, + "::error file=two-changes.md,line=1,title=dprint,endLine=3::File is not formatted. Run `dprint fmt` to fix.%0A%0A@@ -1,3 +1,3 @@%0A-a %0A-b %0A-c %0A+a%0A+b%0A+c%0A(truncated, see the log for the full diff)\n", + ); + assertStringIncludes( + result.stdout, + "::error file=one-change.md,line=1,title=dprint,endLine=6::File is not formatted. Run `dprint fmt` to fix.%0A%0A@@ -1,6 +1,6 @@%0A-a %0A-b %0A-c %0A-d %0A-e %0A-f %0A+a%0A+b%0A+c%0A(truncated, see the log for the full diff)\n", + ); + } finally { + await Deno.remove(workspace, { recursive: true }); + } +}); + Deno.test("makes carriage returns visible and escapes the annotation", async () => { const workspace = await Deno.makeTempDir(); try { @@ -69,7 +131,7 @@ Deno.test("makes carriage returns visible and escapes the annotation", async () assertStringIncludes(result.stdout, "-{\"ok\":true}\\r\n+{ \"ok\": true }\n"); assertStringIncludes( result.stdout, - "::error file=100%25/a%2Cb%3Ac.json,line=1,title=dprint::File is not formatted. Run `dprint fmt` to fix.%0A@@ -1 +1 @@%0A-{\"ok\":true}\\r%0A+{ \"ok\": true }", + "::error file=100%25/a%2Cb%3Ac.json,line=1,title=dprint::File is not formatted. Run `dprint fmt` to fix.%0A%0A@@ -1 +1 @@%0A-{\"ok\":true}\\r%0A+{ \"ok\": true }", ); assertStringIncludes(result.stdout, "Found 1 not formatted file."); } finally { @@ -112,7 +174,7 @@ Deno.test("summarizes a diff that only changes line endings", async () => { "\\ No newline at end of file", "+a", "--", - "::error file=missing-newline.md,line=1,title=dprint::File is not formatted. Run `dprint fmt` to fix.%0A@@ -1 +1 @@%0A-a%0A\\ No newline at end of file%0A+a", + "::error file=missing-newline.md,line=1,title=dprint::File is not formatted. Run `dprint fmt` to fix.%0A%0A@@ -1 +1 @@%0A-a%0A\\ No newline at end of file%0A+a", "Found 3 not formatted files. Run dprint fmt to fix.", "", ]); @@ -179,7 +241,10 @@ Deno.test("clamps the line to 1 for an empty original file", async () => { diff: "--- original\n+++ formatted\n@@ -0,0 +1 @@\n+{}\n", }]); assertEquals(result.code, 0); - assertStringIncludes(result.stdout, "::error file=empty.json,line=1,title=dprint::"); + assertStringIncludes( + result.stdout, + "::error file=empty.json,line=1,title=dprint::File is not formatted. Run `dprint fmt` to fix.%0A%0A@@ -0,0 +1 @@%0A+{}\n", + ); } finally { await Deno.remove(workspace, { recursive: true }); }