diff --git a/.claude/skills/records/SKILL.md b/.claude/skills/records/SKILL.md index d9682b3fb0..3bf9820df9 100644 --- a/.claude/skills/records/SKILL.md +++ b/.claude/skills/records/SKILL.md @@ -32,13 +32,10 @@ or the snapshot file in the diff> Breaking: -Changelog entry: ..md fragment> - Review: Area: syntax printer frontend typechecker lambda codegen runtime rewatch analysis reanalyze gentype tools playground scripts ci docs tests -Changelog: breaking | feature | fix | docs | polish | internal | none Output: unchanged | changed , Formatting: unchanged | changed Formats: none | @@ -61,7 +58,11 @@ Signed-off-by: - **The final block is git trailers**: `Key: value` lines, continuation lines indented by one space, nothing after them. `git interpret-trailers --parse` must read every key. `Signed-off-by` (DCO) is required on every commit. -- **Required keys**: `Area`, `Changelog`, `Output`, `Tests`. `Formatting`, `Formats`, +- **No changelog in the record**: the `changes/..md` + fragment the commit adds is the single source of truth for its changelog + category and entry (see `changes/README.md`). The record repeats neither; + the review page reads the fragment from the diff. +- **Required keys**: `Area`, `Output`, `Tests`. `Formatting`, `Formats`, `PPX` and `Editor` are required whenever the change touches the syntax library, the parsetree, the typed tree or Lambda. Omit the others when they do not apply. @@ -79,7 +80,6 @@ Signed-off-by: | Field | What the record states | | --- | --- | | `Area` | The layers the change touches. They decide who reviews it and which suites apply; a parsetree or typed-tree change also reaches editor analysis. | -| `Changelog` + `Changelog entry` | Whether the change is user-facing, and its category: `breaking` when existing code or tooling must adapt, `internal` when nothing user-visible changes. The entry is one short line ending in the PR link, identical to the fragment's bullet. | | `Output` + `Before`/`After (js)` | The generated JavaScript for the example before and after, and which existing programs compile differently. `unchanged` holds when no committed generated JS changes, or when a rebuild of the test suites is byte-identical. | | `Formatting` + `Before`/`After (rescript)` | Whether `rescript format` rewrites existing code, and whether formatted output parses back to the same tree with its comments. | | `Formats` | The magic numbers bumped in `compiler/ext/config.ml`. Any change to the marshaled parsetree, typed tree (`.cmi`, `.cmt`) or `.cmj` layout, constructor order included, bumps the matching number. | @@ -107,10 +107,13 @@ node scripts/commitRecords.js --out review.html --pr 1234 --title "" < - A revision renders one card. A range renders a summary row, a changelog draft grouped by category, its commits as cards, and the files more than - one commit (or more than one range) changes. -- Each card checks the record against its diff: a changelog category with no - new `changes/` fragment (or a `Changelog entry` that differs from the - fragment's bullet), modified generated `.mjs`, `.cjs` and `.res.js` files under + one commit (or more than one range) changes. The changelog category, the + entries and the draft come from the `changes/` fragments in the diff: a + card shows the fragments its commit adds or changes, and a range's draft + shows the fragments the range leaves, in their last version. +- Each card checks the record against its diff: a `changes/` fragment that + `yarn changelog:check` rejects (such as one still carrying the placeholder + PR link), an edit to `CHANGELOG.md`, modified generated `.mjs`, `.cjs` and `.res.js` files under `Output: unchanged` (new ones from new tests are counted, not flagged), a magic number bumped in `compiler/ext/config.ml` that `Formats` does not name, a parsetree change with no ast-mapping fixture, a diff --git a/changes/semantic-diff-fragments.internal.md b/changes/semantic-diff-fragments.internal.md new file mode 100644 index 0000000000..2970db314e --- /dev/null +++ b/changes/semantic-diff-fragments.internal.md @@ -0,0 +1 @@ +- Read the semantic diff page's changelog from the `changes/` fragments, the single source of truth, instead of commit message trailers. https://github.com/rescript-lang/rescript/pull/8881 diff --git a/scripts/commitRecords.js b/scripts/commitRecords.js index 8b7aea0def..0522fd5f5a 100644 --- a/scripts/commitRecords.js +++ b/scripts/commitRecords.js @@ -5,7 +5,9 @@ /* Renders commit records as a review page. A record is a commit message (or a * note on refs/notes/records, which takes precedence) in the format of * .claude/skills/records/SKILL.md: a subject, prose sections, code sections - * such as "Before (js):" and git trailers. + * such as "Before (js):" and git trailers. The changelog comes from the + * changes/..md fragments the commits add, not from the + * record. * * node scripts/commitRecords.js --out review.html ... * node scripts/commitRecords.js --out review.html ..... @@ -15,6 +17,7 @@ import { execFileSync } from "node:child_process"; import { writeFileSync } from "node:fs"; +import { parseFragment } from "./changelog.js"; const REPO_URL = "https://github.com/rescript-lang/rescript"; @@ -156,10 +159,12 @@ function codeLines(lines, lang, other, mark) { /** * @typedef {{ name: string, lang: string, text: string[], block: string[] }} Section * @typedef {{ status: string, path: string, added: number, removed: number }} FileChange + * @typedef {{ path: string, category: string, text: string, error: string }} Fragment * @typedef {{ * sha: string, author: string, date: string, subject: string, * sections: Section[], trailers: Map, message: string, - * coauthors: string[], files: FileChange[], patch: string, noted: boolean + * coauthors: string[], files: FileChange[], fragments: Fragment[], + * patch: string, noted: boolean * }} CommitRecord */ @@ -188,6 +193,49 @@ function pushProse(sections) { return s; } +/** @param {string} path */ +const isFragment = path => + /^changes\/[^/]+\.md$/.test(path) && path !== "changes/README.md"; + +/** + * The changelog fragments a commit adds or changes, as they stand in its tree. + * A fragment that fails validation (such as one with the placeholder PR link) + * keeps the category from its name and records the error. + * @param {string} rev + * @param {FileChange[]} files + * @returns {Fragment[]} + */ +function fragmentsOf(rev, files) { + return files + .filter(f => f.status !== "D" && isFragment(f.path)) + .map(f => { + const name = f.path.slice("changes/".length); + const content = git(["show", `${rev}:${f.path}`]); + try { + const { category, text } = parseFragment(name, content); + return { path: f.path, category, text, error: "" }; + } catch (e) { + return { + path: f.path, + category: /\.([a-z]+)\.md$/.exec(name)?.[1] ?? "?", + text: content.trim(), + error: e instanceof Error ? e.message : String(e), + }; + } + }); +} + +/** + * The bullets of a fragment, without their "- " and with continuation lines + * joined. + * @param {string} text + */ +const bullets = text => + text + .split(/\n(?=- )/) + .map(b => b.replace(/^-\s+/, "").replace(/\n\s+/g, " ")) + .filter(Boolean); + /** @param {string} rev */ function readRecord(rev) { let text = ""; @@ -296,14 +344,13 @@ function readRecord(rev) { ), ], files: [...files.values()], + fragments: fragmentsOf(rev, [...files.values()]), patch: git([ "show", "--format=", "-U0", rev, "--", - "CHANGELOG.md", - "changes", "compiler/ext/config.ml", ]), }; @@ -362,39 +409,17 @@ function checks(r) { /** @type {[boolean, string][]} */ const out = []; const has = (/** @type {RegExp} */ re) => r.files.some(f => re.test(f.path)); - const category = r.trailers.get("Changelog"); - const changelogTouched = has(/^CHANGELOG\.md$|^changes\/.*\.md$/); - if (category !== undefined) { - if (category === "none") - out.push([ - !changelogTouched, - changelogTouched - ? "Changelog: none, but the diff changes the changelog" - : "No changelog entry, as recorded", - ]); - else if (!changelogTouched) - out.push([ - false, - `Changelog: ${category}, but the diff adds no changelog entry`, - ]); - else { - const entry = r.sections.find(s => s.name === "Changelog entry"); - const added = r.patch - .split("\n") - .filter(l => l.startsWith("+") && !l.startsWith("+++")) - .join("\n"); - const text = entry - ? [...entry.text, ...entry.block].join(" ").trim() - : ""; - if (text) - out.push( - added.replace(/\s+/g, " ").includes(text.replace(/\s+/g, " ")) - ? [true, "Changelog entry matches the line the diff adds"] - : [false, "Changelog entry differs from the line the diff adds"], - ); - else out.push([true, `Changelog: ${category}; the diff adds an entry`]); - } - } else out.push([false, "No Changelog trailer"]); + for (const f of r.fragments) + out.push( + f.error + ? [false, `${f.path}: ${f.error.replace(/^[^:]*: /, "")}`] + : [true, `Changelog fragment ${f.path}`], + ); + if (has(/^CHANGELOG\.md$/)) + out.push([ + false, + "Edits CHANGELOG.md; a changes/ fragment carries the entry outside release PRs", + ]); const testsValue = r.trailers.get("Tests"); const testFiles = r.files.filter(f => @@ -478,9 +503,7 @@ function checks(r) { /** @type {Record} */ const CATEGORIES = { breaking: ["๐Ÿ’ฅ", "Breaking change"], - spec: ["๐Ÿ‘“", "Spec compliance"], feature: ["๐Ÿš€", "New feature"], - bugfix: ["๐Ÿ›", "Bug fix"], fix: ["๐Ÿ›", "Bug fix"], docs: ["๐Ÿ“", "Documentation"], polish: ["๐Ÿ’…", "Polish"], @@ -595,7 +618,7 @@ function renderSections(sections) { const out = []; for (let i = 0; i < sections.length; i++) { const s = sections[i]; - if (["Breaking", "Changelog entry", "Review"].includes(s.name)) continue; + if (["Breaking", "Review"].includes(s.name)) continue; const next = sections[i + 1]; if ( s.name === "Before" && @@ -668,7 +691,7 @@ const langName = lang => /** @param {CommitRecord} r */ function card(r) { - const category = r.trailers.get("Changelog") ?? ""; + const categories = [...new Set(r.fragments.map(f => f.category))]; const pr = ( r.trailers.get("PR") ?? /\(#(\d+)\)\s*$/.exec(r.subject)?.[0] ?? @@ -676,14 +699,14 @@ function card(r) { ).replace(/\D/g, ""); const subject = r.subject.replace(/\s*\(#\d+\)\s*$/, ""); const out = [ - `
`, + `
`, ]; out.push( `
${r.sha} ยท ${esc(r.author)} ยท ${esc(r.date)}${ pr ? ` ยท #${pr} discussion โ†—` : "" }${r.noted ? ' ยท note' : ""}
` + `

${inline(subject)}

` + - `
${category ? changelogPill(category) : ""}${words( + `
${categories.map(changelogPill).join("")}${words( r, "Area", ) @@ -696,10 +719,9 @@ function card(r) { ``, ); out.push(renderSections(r.sections)); - const entry = r.sections.find(s => s.name === "Changelog entry"); - if (entry) + for (const f of r.fragments) out.push( - `
${category ? changelogPill(category) : ""}${inline([...entry.text, ...entry.block].join(" "))}
`, + `
${changelogPill(f.category)}${bullets(f.text).map(inline).join("
")}
`, ); const reviews = r.sections.filter(s => s.name === "Review"); if (reviews.length) @@ -791,16 +813,30 @@ function group(arg) { }; } +/** + * The fragments a range leaves in changes/: the last version of each one its + * commits add or change, minus those a later commit deletes. + * @param {Group} g + */ +function rangeFragments(g) { + /** @type {Map} */ + const byPath = new Map(); + for (const r of g.records) { + for (const f of r.files) if (f.status === "D") byPath.delete(f.path); + for (const f of r.fragments) byPath.set(f.path, f); + } + return [...byPath.values()]; +} + /** @param {Group[]} groups */ function summary(groups) { const ranges = groups.filter(g => g.range); if (!ranges.length) return ""; const rows = ranges.map(g => { const cats = new Map(); - for (const r of g.records) { - const c = r.trailers.get("Changelog") ?? "?"; - cats.set(c, (cats.get(c) ?? 0) + 1); - } + for (const f of rangeFragments(g)) + cats.set(f.category, (cats.get(f.category) ?? 0) + 1); + if (!cats.size) cats.set("none", 1); const areas = [...new Set(g.records.flatMap(r => words(r, "Area")))]; const js = g.records.filter(r => /^changed/.test(r.trailers.get("Output") ?? ""), @@ -840,15 +876,11 @@ function summary(groups) { /** @type {Map} */ const draft = new Map(); for (const g of ranges) - for (const r of g.records) { - const c = r.trailers.get("Changelog"); - const entry = r.sections.find(s => s.name === "Changelog entry"); - if (c && c !== "none" && entry) - draft.set(c, [ - ...(draft.get(c) ?? []), - [...entry.text, ...entry.block].join(" ").replace(/^-\s+/, ""), - ]); - } + for (const f of rangeFragments(g)) + draft.set(f.category, [ + ...(draft.get(f.category) ?? []), + ...bullets(f.text), + ]); const order = Object.keys(CATEGORIES); return ( `

Summary

${rows.join("")}
RangeCommitsChangelogAreasJS output changesFormatsFailed checks
` + @@ -906,7 +938,7 @@ code { font:12.5px/1.5 var(--mono); background:var(--code-bg); padding:1px 4px; .chips { display:flex; flex-wrap:wrap; gap:6px; align-items:center } .chip { font:500 12px/1 var(--body); border:1px solid var(--rule); border-radius:99px; padding:5px 9px; color:var(--muted) } .pill { font:600 12px/1 var(--body); padding:6px 10px; border-radius:99px; white-space:nowrap; background:var(--same-bg); color:var(--same) } -.pill.cat-breaking { background:var(--accent); color:#fff } .pill.cat-bugfix, .pill.cat-fix { background:var(--warn-bg); color:var(--warn) } +.pill.cat-breaking { background:var(--accent); color:#fff } .pill.cat-fix { background:var(--warn-bg); color:var(--warn) } .pill.cat-feature { background:var(--ok-bg); color:var(--ok) } .pill.cat-polish { background:var(--diff-bg); color:var(--diff) } .banner { background:var(--accent-soft); border-left:4px solid var(--accent); padding:10px 14px; border-radius:6px; margin:4px 0 } .banner strong { color:var(--accent); margin-right:6px; text-transform:uppercase; font-size:12px; letter-spacing:.06em } diff --git a/scripts/commitRecords.test.js b/scripts/commitRecords.test.js index 4517eea3ff..510a458d72 100644 --- a/scripts/commitRecords.test.js +++ b/scripts/commitRecords.test.js @@ -23,10 +23,7 @@ After (js): Source: bsc before and after this commit. -Changelog entry: - Compile \`x + 0\` to \`x\`. https://github.com/rescript-lang/rescript/pull/1234 - Area: codegen tests -Changelog: polish Output: changed integer + 0; one snapshot Formats: none Tests: +tests/tests/src/a_test.mjs ~tests/tests/src/b_test.mjs @@ -93,7 +90,11 @@ test("renders a record and checks it against the diff", async () => { has(html, ''); has(html, ''); has(html, '

Source'); - has(html, "Changelog entry matches the line the diff adds"); + has(html, "Changelog fragment changes/identity.polish.md"); + has( + html, + '

๐Ÿ’… PolishCompile x + 0', + ); has(html, "1 committed generated JS file changes; 1 new"); has(html, "Formats: none"); has(html, "tests/tests/src/"); @@ -115,7 +116,7 @@ test("flags a record that disagrees with its diff", async () => { "commit", "-q", "-m", - "Change output\n\nArea: codegen\nChangelog: fix\nOutput: unchanged", + "Change output\n\nArea: codegen\nOutput: unchanged", ); }, ["--pr", "42", "--title", "Fix & more", "HEAD"], @@ -125,7 +126,6 @@ test("flags a record that disagrees with its diff", async () => { '

#42 Fix <output> & more

', ); has(html, "#42 Fix <output> & more ยท Semantic Diff"); - has(html, "Changelog: fix, but the diff adds no changelog entry"); has(html, "Output: unchanged, but 1 committed generated JS file changes"); has(html, "No Tests trailer"); }); @@ -142,7 +142,7 @@ test("accepts Tests: none with a reason, and flags it against test files", async "commit", "-q", "-m", - "Reword the readme\n\nArea: docs\nChangelog: none\nOutput: unchanged\nTests: none, because only prose changes", + "Reword the readme\n\nArea: docs\nOutput: unchanged\nTests: none, because only prose changes", ); await write(dir, { "tests/tests/src/c_test.res": "let x = 1\n" }); git("add", "."); @@ -150,7 +150,7 @@ test("accepts Tests: none with a reason, and flags it against test files", async "commit", "-q", "-m", - "Add a test\n\nArea: tests\nChangelog: none\nOutput: unchanged\nTests: none, because it is a fixture", + "Add a test\n\nArea: tests\nOutput: unchanged\nTests: none, because it is a fixture", ); }, ["HEAD~2..HEAD"], @@ -161,4 +161,45 @@ test("accepts Tests: none with a reason, and flags it against test files", async 'nonebecause only prose changes', ); has(html, "Tests: none, but the diff changes 1 test file"); + has(html, 'No changelog'); +}); + +test("takes the changelog from the fragments a range leaves", async () => { + const link = "https://github.com/rescript-lang/rescript/pull"; + const html = await render( + async (git, dir) => { + await write(dir, { "CHANGELOG.md": "# Changelog\n" }); + git("add", "."); + git("commit", "-q", "-m", "base"); + await write(dir, { + "changes/crash.fix.md": `- Fix a crash. ${link}/XXXX\n`, + "changes/gone.feature.md": `- Add a thing. ${link}/7\n`, + }); + git("add", "."); + git("commit", "-q", "-m", "Draft\n\nArea: compiler"); + await write(dir, { + "changes/crash.fix.md": `- Fix a crash\n in the printer. ${link}/8\n- Fix another. ${link}/8\n`, + "CHANGELOG.md": "# Changelog\n\n- Edited.\n", + }); + git("rm", "-q", "changes/gone.feature.md"); + git("add", "."); + git("commit", "-q", "-m", "Link the PR\n\nArea: compiler"); + }, + ["HEAD~2..HEAD"], + ); + has( + html, + "changes/crash.fix.md: replace the placeholder https://github.com/rescript-lang/rescript/pull/XXXX with the PR link", + ); + has(html, "Changelog fragment changes/gone.feature.md"); + has( + html, + "Edits CHANGELOG.md; a changes/ fragment carries the entry outside release PRs", + ); + has(html, '๐Ÿ› Bug fix'); + has( + html, + '
๐Ÿ› Bug fix
  • Fix a crash in the printer. #8
  • Fix another.', + ); + assert.ok(!html.includes("
  • Add a thing."), "deleted fragment in draft"); });