Public: anyone can read this; the crew can change it.
Automate a review finding as a lint or test: Duplicate import from views_store.ts; merge into one. #11593
Check · Review · Land · Live — No review yet.
scout/forge:scout/11516-automate-a-review-finding-as-a-lint-or-t → main 1 commit fork
3 files
changed +247
−0
scripts/dup-imports-allow.json added +134
| @@ -0,0 +1,134 @@ | ||
| 1 | [ | |
| 2 | "src/actions/apps.ts", | |
| 3 | "src/actions/asks_decide.ts", | |
| 4 | "src/actions/fleet.ts", | |
| 5 | "src/actions/holds.ts", | |
| 6 | "src/actions/pulls.ts", | |
| 7 | "src/actions/repos.ts", | |
| 8 | "src/app.ts", | |
| 9 | "src/apps/runtime.ts", | |
| 10 | "src/apps/scaffold.ts", | |
| 11 | "src/checks/service.ts", | |
| 12 | "src/core/reconcile.ts", | |
| 13 | "src/crew/autonomy.ts", | |
| 14 | "src/crew/dispatch.ts", | |
| 15 | "src/crew/runner.ts", | |
| 16 | "src/crew/vision.ts", | |
| 17 | "src/http/api.ts", | |
| 18 | "src/http/api/admin.ts", | |
| 19 | "src/http/api/app.ts", | |
| 20 | "src/http/api/upstream.ts", | |
| 21 | "src/http/rp.ts", | |
| 22 | "src/http/server.ts", | |
| 23 | "src/tui/forge.ts", | |
| 24 | "src/tui/screens/dashboard.ts", | |
| 25 | "src/tui/screens/display.ts", | |
| 26 | "src/tui/screens/external.ts", | |
| 27 | "src/tui/screens/forges.ts", | |
| 28 | "src/tui/screens/forks.ts", | |
| 29 | "src/tui/screens/issue.ts", | |
| 30 | "src/tui/screens/packs.ts", | |
| 31 | "src/tui/screens/system.ts", | |
| 32 | "src/tui/session.ts", | |
| 33 | "src/web/chrome.ts", | |
| 34 | "src/web/pages/board.ts", | |
| 35 | "src/web/pages/flow-panel.ts", | |
| 36 | "src/web/pages/following.ts", | |
| 37 | "src/web/pages/forges.ts", | |
| 38 | "src/web/pages/repo.ts", | |
| 39 | "src/web/pages/runs.ts", | |
| 40 | "src/web/pages/settings.ts", | |
| 41 | "src/web/pages/site.ts", | |
| 42 | "src/web/pages/sources.ts", | |
| 43 | "src/web/pages/talk.ts", | |
| 44 | "src/web/pages/teams.ts", | |
| 45 | "src/web/pages/thread.ts", | |
| 46 | "src/web/studio/home.ts", | |
| 47 | "test/api-forge-view.test.ts", | |
| 48 | "test/api-reader-marks.test.ts", | |
| 49 | "test/app-variants.test.ts", | |
| 50 | "test/apps-build-preview.test.ts", | |
| 51 | "test/apps-crew.test.ts", | |
| 52 | "test/apps-measure.test.ts", | |
| 53 | "test/apps-proxy-unavailable.test.ts", | |
| 54 | "test/apps.test.ts", | |
| 55 | "test/asks-bulk.test.ts", | |
| 56 | "test/asks-counts.test.ts", | |
| 57 | "test/autonomy.test.ts", | |
| 58 | "test/bets-card-fit.test.ts", | |
| 59 | "test/bets.test.ts", | |
| 60 | "test/billing-never-deletes.test.ts", | |
| 61 | "test/board-pins.test.ts", | |
| 62 | "test/board-windows.test.ts", | |
| 63 | "test/boards-query.test.ts", | |
| 64 | "test/boards-starting.test.ts", | |
| 65 | "test/builds-rows.test.ts", | |
| 66 | "test/business-plan.test.ts", | |
| 67 | "test/check-order-dependent.test.ts", | |
| 68 | "test/checks-placement.test.ts", | |
| 69 | "test/child-credit.test.ts", | |
| 70 | "test/client-list.test.ts", | |
| 71 | "test/conversation-talk.test.ts", | |
| 72 | "test/crew-direction.test.ts", | |
| 73 | "test/crew-loop.test.ts", | |
| 74 | "test/crew-network.test.ts", | |
| 75 | "test/crew-pin-hook.test.ts", | |
| 76 | "test/crew-sandbox-escape.test.ts", | |
| 77 | "test/crew-sandbox-real.test.ts", | |
| 78 | "test/crew-sandbox.test.ts", | |
| 79 | "test/everyday.test.ts", | |
| 80 | "test/explore-unknown-words.test.ts", | |
| 81 | "test/explore.test.ts", | |
| 82 | "test/fleet-graph-href.test.ts", | |
| 83 | "test/flow-card.test.ts", | |
| 84 | "test/following-cache.test.ts", | |
| 85 | "test/following.test.ts", | |
| 86 | "test/forge-budget-verdict.test.ts", | |
| 87 | "test/guests.test.ts", | |
| 88 | "test/home-builders-idle.test.ts", | |
| 89 | "test/keys.test.ts", | |
| 90 | "test/knobs.test.ts", | |
| 91 | "test/machine-rc.test.ts", | |
| 92 | "test/machine-shell.test.ts", | |
| 93 | "test/mcp-org-teams-waiting.test.ts", | |
| 94 | "test/mcp-releases-card.test.ts", | |
| 95 | "test/migrate.test.ts", | |
| 96 | "test/no-polling.test.ts", | |
| 97 | "test/notify.test.ts", | |
| 98 | "test/now-links.test.ts", | |
| 99 | "test/orgs.test.ts", | |
| 100 | "test/origin-pull-state.test.ts", | |
| 101 | "test/packs-up.test.ts", | |
| 102 | "test/phase-profile.test.ts", | |
| 103 | "test/planner-closes-pull.test.ts", | |
| 104 | "test/preview-looks.test.ts", | |
| 105 | "test/preview-spec.test.ts", | |
| 106 | "test/private-app.test.ts", | |
| 107 | "test/proposal-run.test.ts", | |
| 108 | "test/release-boot.test.ts", | |
| 109 | "test/repo-transfer.test.ts", | |
| 110 | "test/request-access-limits.test.ts", | |
| 111 | "test/resilience.test.ts", | |
| 112 | "test/search-is-palette.test.ts", | |
| 113 | "test/shards-run.test.ts", | |
| 114 | "test/signup-password.test.ts", | |
| 115 | "test/spawn-cell.test.ts", | |
| 116 | "test/spawn-self-use.test.ts", | |
| 117 | "test/starter-crew.test.ts", | |
| 118 | "test/system-feature-health.test.ts", | |
| 119 | "test/system-telemetry.test.ts", | |
| 120 | "test/talk-conversations.test.ts", | |
| 121 | "test/talk-steps.test.ts", | |
| 122 | "test/tree-window.test.ts", | |
| 123 | "test/tui-about-detach.test.ts", | |
| 124 | "test/tui-archive.test.ts", | |
| 125 | "test/tui-ask-data.test.ts", | |
| 126 | "test/tui-claim-start.test.ts", | |
| 127 | "test/tui-data.test.ts", | |
| 128 | "test/tui-transfer-repo.test.ts", | |
| 129 | "test/upstream_follow.test.ts", | |
| 130 | "test/vitals.test.ts", | |
| 131 | "test/web.test.ts", | |
| 132 | "test/webhook-failing.test.ts", | |
| 133 | "test/week-sprint.test.ts" | |
| 134 | ] |
scripts/dup-imports.ts added +82
| @@ -0,0 +1,82 @@ | ||
| 1 | /** | |
| 2 | * Rule `duplicate-import`: one module is imported on one line (#11516). | |
| 3 | * | |
| 4 | * Reviewers asked five times in a week to merge two `import … from "x"` lines | |
| 5 | * into one. This names them. A `import type` and a value `import` of the same | |
| 6 | * module are two kinds and may stay apart; a namespace import (`* as`) and a | |
| 7 | * bare side-effect import cannot merge, so they are not counted. | |
| 8 | */ | |
| 9 | ||
| 10 | import { readdirSync, readFileSync, statSync } from "node:fs"; | |
| 11 | import { join } from "node:path"; | |
| 12 | ||
| 13 | export const RULE = "duplicate-import"; | |
| 14 | ||
| 15 | const IMPORT = /^import\s+(type\s+)?([^;]*?)\s+from\s+["']([^"']+)["']/gms; | |
| 16 | ||
| 17 | /** One line per module imported twice (or more) in the same kind, in `source`. */ | |
| 18 | export function duplicateImports(file: string, source: string): string[] { | |
| 19 | const seen = new Map<string, number>(); | |
| 20 | const problems: string[] = []; | |
| 21 | for (const m of source.matchAll(IMPORT)) { | |
| 22 | const clause = m[2] ?? ""; | |
| 23 | if (clause.includes("* as")) continue; | |
| 24 | const key = `${m[1] ? "type " : ""}${m[3]}`; | |
| 25 | const line = source.slice(0, m.index).split("\n").length; | |
| 26 | const first = seen.get(key); | |
| 27 | if (first === undefined) seen.set(key, line); | |
| 28 | else | |
| 29 | problems.push( | |
| 30 | `${file}:${line}: ${RULE}: "${m[3]}" is already imported on line ${first}; merge into one import`, | |
| 31 | ); | |
| 32 | } | |
| 33 | return problems; | |
| 34 | } | |
| 35 | ||
| 36 | function walk(dir: string, out: string[]): void { | |
| 37 | for (const name of readdirSync(dir)) { | |
| 38 | if (name === "node_modules" || name.startsWith(".")) continue; | |
| 39 | const p = join(dir, name); | |
| 40 | if (statSync(p).isDirectory()) walk(p, out); | |
| 41 | else if (/\.(ts|tsx)$/.test(name)) out.push(p); | |
| 42 | } | |
| 43 | } | |
| 44 | ||
| 45 | /** Every duplicate import in src/, scripts/ and test/ under `root`. */ | |
| 46 | export function repoDuplicateImports(root = "."): string[] { | |
| 47 | const files: string[] = []; | |
| 48 | for (const d of ["src", "scripts", "test"]) { | |
| 49 | try { | |
| 50 | walk(join(root, d), files); | |
| 51 | } catch {} | |
| 52 | } | |
| 53 | return files | |
| 54 | .sort() | |
| 55 | .flatMap((f) => duplicateImports(f.replace(`${root}/`, ""), readFileSync(f, "utf8"))); | |
| 56 | } | |
| 57 | ||
| 58 | /** Files that predate the rule (`scripts/dup-imports-allow.json`); it only shrinks. */ | |
| 59 | export function allowed(root = "."): string[] { | |
| 60 | return JSON.parse(readFileSync(join(root, "scripts/dup-imports-allow.json"), "utf8")); | |
| 61 | } | |
| 62 | ||
| 63 | /** | |
| 64 | * What fails: a duplicate in a file not on the allowlist, and an allowlisted | |
| 65 | * file that is clean now (delete its entry, so the list only shrinks). | |
| 66 | */ | |
| 67 | export function judge(problems: readonly string[], allow: readonly string[]): string[] { | |
| 68 | const ok = new Set(allow); | |
| 69 | const dirty = new Set(problems.map((p) => p.split(":")[0] as string)); | |
| 70 | return [ | |
| 71 | ...problems.filter((p) => !ok.has(p.split(":")[0] as string)), | |
| 72 | ...allow | |
| 73 | .filter((f) => !dirty.has(f)) | |
| 74 | .map((f) => `${f}: ${RULE}: no duplicate import left; remove it from scripts/dup-imports-allow.json`), | |
| 75 | ]; | |
| 76 | } | |
| 77 | ||
| 78 | if (import.meta.main) { | |
| 79 | const problems = judge(repoDuplicateImports(), allowed()); | |
| 80 | for (const p of problems) console.error(p); | |
| 81 | process.exit(problems.length ? 1 : 0); | |
| 82 | } |
test/dup-imports.test.ts added +31
| @@ -0,0 +1,31 @@ | ||
| 1 | import { expect, test } from "bun:test"; | |
| 2 | import { allowed, duplicateImports, judge, repoDuplicateImports } from "../scripts/dup-imports.ts"; | |
| 3 | ||
| 4 | test("two imports of one module are named, with the rule and both lines", () => { | |
| 5 | const src = `import { a } from "./x.ts";\nimport { b } from "./x.ts";\n`; | |
| 6 | expect(duplicateImports("f.ts", src)).toEqual([ | |
| 7 | `f.ts:2: duplicate-import: "./x.ts" is already imported on line 1; merge into one import`, | |
| 8 | ]); | |
| 9 | }); | |
| 10 | ||
| 11 | test("multi-line imports count, and different modules do not", () => { | |
| 12 | const src = `import {\n a,\n b,\n} from "./x.ts";\nimport { c } from "./y.ts";\nimport { d } from "./x.ts";\n`; | |
| 13 | expect(duplicateImports("f.ts", src)).toHaveLength(1); | |
| 14 | expect(duplicateImports("f.ts", `import { a } from "./x.ts";\nimport { c } from "./y.ts";\n`)).toEqual([]); | |
| 15 | }); | |
| 16 | ||
| 17 | test("a type import beside a value import, namespaces and side effects are left alone", () => { | |
| 18 | const src = `import type { A } from "./x.ts";\nimport { a } from "./x.ts";\nimport * as n from "./x.ts";\nimport "./x.ts";\n`; | |
| 19 | expect(duplicateImports("f.ts", src)).toEqual([]); | |
| 20 | }); | |
| 21 | ||
| 22 | test("a new duplicate fails; a cleaned file must leave the allowlist", () => { | |
| 23 | const p = `new.ts:2: duplicate-import: "x" is already imported on line 1; merge into one import`; | |
| 24 | expect(judge([p], [])).toEqual([p]); | |
| 25 | expect(judge([p], ["new.ts"])).toEqual([]); | |
| 26 | expect(judge([], ["old.ts"])[0]).toContain("remove it from scripts/dup-imports-allow.json"); | |
| 27 | }); | |
| 28 | ||
| 29 | test("the repository has no duplicate import beyond the allowlist (rule duplicate-import)", () => { | |
| 30 | expect(judge(repoDuplicateImports(), allowed())).toEqual([]); | |
| 31 | }); |