Public: anyone can read this; the crew can change it.
Automate a review finding as a lint or test: Smaller points are in the line comments. #8886
Check · Review · Land · Live done — Landed and live in production.
mason/forge:mason/8877-automate-a-review-finding-as-a-lint-or-t → main 1 commit fork
4 files
changed +49
−6
src/cluster/jobs.ts +1 −2
| @@ -219,8 +219,7 @@ export async function runJob(spec: JobSpec, opts: JobOptions = {}): Promise<JobR | ||
| 219 | 219 | else if (now() >= deadline) { |
| 220 | 220 | const pod = await findPod().catch(() => undefined); |
| 221 | 221 | return { exitCode: null, lines: await logLines(pod?.metadata?.name), timedOut: true, ...nodeOf(pod) }; |
| 222 | } | |
| 223 | else await sleep(pollMs); | |
| 222 | } else await sleep(pollMs); | |
| 224 | 223 | } |
| 225 | 224 | const pod = await findPod(); |
| 226 | 225 | // A failed Job with no pod, or a pod the kubelet evicted, never ran its |
src/http/server.ts +1 −2
| @@ -1456,8 +1456,7 @@ async function withGitAccess( | ||
| 1456 | 1456 | else if (created.error === "storage_limit") { |
| 1457 | 1457 | const ownerId = resolveOwner(ctx.app, ctx.auth.user, ctx.params.owner!) ?? ctx.auth.user.id; |
| 1458 | 1458 | return gitRefused(ctx, storageRefusedText(storageSentence(ctx.app, ownerId) ?? "at the limit")); |
| 1459 | } | |
| 1460 | else if (created.error === "not_member") | |
| 1459 | } else if (created.error === "not_member") | |
| 1461 | 1460 | return gitRefused(ctx, `Not made: ${NOT_A_MEMBER}`); |
| 1462 | 1461 | else if (created.error === "repository_limit") |
| 1463 | 1462 | return gitRefused( |
src/tui/screens/dashboard.ts +1 −2
| @@ -953,8 +953,7 @@ export function dashboardScreen(f: Session, ctx: Ctx): Screen { | ||
| 953 | 953 | if (focus === "repos") { |
| 954 | 954 | if (exploring()) openExplored(exploredRows()[at("repos").index]); |
| 955 | 955 | else openRepo(shown()[at("repos").index]); |
| 956 | } | |
| 957 | else if (focus === "onyou") openAssigned(onYou[at("onyou").index]); | |
| 956 | } else if (focus === "onyou") openAssigned(onYou[at("onyou").index]); | |
| 958 | 957 | else openPin(pins[at("machine").index]); |
| 959 | 958 | }; |
| 960 | 959 |
test/review-dangling-else.test.ts added +46
| @@ -0,0 +1,46 @@ | ||
| 1 | /** | |
| 2 | * Review rule: `else` follows its closing brace on the same line. | |
| 3 | * | |
| 4 | * Reviewers asked for this again and again (#7612 and others): a `}` alone | |
| 5 | * on a line with `else` on the next is mis-formatted. This test says it for | |
| 6 | * them. Browser scripts that live inside template strings are not checked: | |
| 7 | * `src/web/client.ts` and `src/apps/toolbar.ts` keep one branch per line on | |
| 8 | * purpose, and the text there is not TypeScript. | |
| 9 | */ | |
| 10 | ||
| 11 | import { expect, test } from "bun:test"; | |
| 12 | import { readdirSync, readFileSync } from "node:fs"; | |
| 13 | import { join } from "node:path"; | |
| 14 | ||
| 15 | const root = new URL("..", import.meta.url).pathname; | |
| 16 | const SKIPPED = new Set(["src/web/client.ts", "src/apps/toolbar.ts"]); | |
| 17 | ||
| 18 | function sources(dir: string): string[] { | |
| 19 | return readdirSync(join(root, dir), { withFileTypes: true }).flatMap((e) => { | |
| 20 | const path = `${dir}/${e.name}`; | |
| 21 | if (e.isDirectory()) return sources(path); | |
| 22 | return e.name.endsWith(".ts") && !SKIPPED.has(path) ? [path] : []; | |
| 23 | }); | |
| 24 | } | |
| 25 | ||
| 26 | /** `file:line` of every `else` that starts a line right after a lone `}`. */ | |
| 27 | export function danglingElses(path: string, text: string): string[] { | |
| 28 | const lines = text.split("\n"); | |
| 29 | const found: string[] = []; | |
| 30 | for (let i = 1; i < lines.length; i++) { | |
| 31 | if (/^\s*else\b/.test(lines[i]!) && /^\s*\}\s*$/.test(lines[i - 1]!)) found.push(`${path}:${i + 1}`); | |
| 32 | } | |
| 33 | return found; | |
| 34 | } | |
| 35 | ||
| 36 | test("the rule finds a dangling else", () => { | |
| 37 | expect(danglingElses("x.ts", "if (a) {\n b();\n}\nelse c();\n")).toEqual(["x.ts:4"]); | |
| 38 | expect(danglingElses("x.ts", "if (a) {\n b();\n} else c();\n")).toEqual([]); | |
| 39 | }); | |
| 40 | ||
| 41 | test("review rule: write `} else`, not `}` and `else` on separate lines", () => { | |
| 42 | const bad = ["src", "scripts"].flatMap((dir) => | |
| 43 | sources(dir).flatMap((path) => danglingElses(path, readFileSync(join(root, path), "utf8"))), | |
| 44 | ); | |
| 45 | expect(bad, "rule: `else` goes on the same line as the closing `}`; found at these lines").toEqual([]); | |
| 46 | }); |
new above
offline