Skip to content
Merged
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand Down
185 changes: 136 additions & 49 deletions scripts/annotate.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand All @@ -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) {
Expand All @@ -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;
Expand All @@ -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] += "<no newline>";
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] += "<no newline>";
}
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) {
Expand All @@ -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")
Expand Down
77 changes: 71 additions & 6 deletions scripts/annotate_test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ async function runAnnotate(workspace: string, entries: Record<string, unknown>[]
};
}

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, [{
Expand All @@ -41,15 +41,15 @@ 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",
"@@ -1 +1 @@",
"-{\"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.",
"",
]);
Expand All @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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.",
"",
]);
Expand Down Expand Up @@ -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 });
}
Expand Down
Loading