Skip to content

Commit 99227fb

Browse files
committed
ci: a package file's comment share may not rise
1 parent 3194c2a commit 99227fb

5 files changed

Lines changed: 379 additions & 3 deletions

File tree

‎.github/workflows/comments.yml‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,8 +33,13 @@ jobs:
3333
# The commented-out-code rule asks the TypeScript parser, a root dependency.
3434
- run: bash scripts/ci/install-workspace-dependencies.sh --ignore-scripts
3535
- name: Test the checker
36-
run: node --test scripts/check-comment-citations.test.mjs
36+
run: node --test scripts/check-comment-citations.test.mjs scripts/comment-ratchet.test.mjs
3737
- name: Check the comments this diff touched
3838
env:
3939
COMMENT_CHECK_BASE: ${{ github.event.pull_request.base.sha }}
4040
run: node scripts/check-comment-citations.mjs
41+
- name: A package file's comment share may not rise
42+
if: ${{ !cancelled() }}
43+
env:
44+
COMMENT_CHECK_BASE: ${{ github.event.pull_request.base.sha }}
45+
run: node scripts/comment-ratchet.mjs

‎CONTRIBUTING.md‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,9 @@ The `Comments` check (`scripts/check-comment-citations.mjs`) grades the comments
8080
- **No commented-out code.** Delete it; git keeps it.
8181
- **No block over 40 lines.** Cut it to the why and the invariant. A block that must stay whole (a licence, a diagram, a protocol table) starts with `comment-length: <reason>`.
8282

83-
Only comment blocks holding a line your PR added can fail. Findings elsewhere in a file you touched are printed as warnings, and fixing one while you are there is welcome. To grade files by hand, pass their paths: `node scripts/check-comment-citations.mjs path/to/file.ts`.
83+
- **A package file's comment share may not rise** (`scripts/comment-ratchet.mjs`). For source under `packages/*/src` (tests excluded), a file you change may not end up with a higher share of comment lines than it had where your branch forked, unless you only deleted code. It may not gain a new comment block over 12 lines, and a new file may not start above its package's share. Move the explanation into a name, a type or the PR description.
84+
85+
Only comment blocks holding a line your PR added can fail the citation and block rules. Findings elsewhere in a file you touched are printed as warnings, and fixing one while you are there is welcome. To grade files by hand, pass their paths: `node scripts/check-comment-citations.mjs path/to/file.ts`.
8486

8587
## Adding Registry Items (Blocks & Components)
8688

‎package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252
"player:perf": "bun run --filter @hyperframes/player perf",
5353
"format:check": "oxfmt --check .",
5454
"knip": "knip",
55-
"test:scripts": "node --import tsx --test scripts/animejs-v4-guidance.test.mjs scripts/check-tracked-artifacts.test.mjs scripts/check-registry-set-delta.test.mjs scripts/check-no-main-deletions.test.mjs scripts/check-pr-captures.test.mjs scripts/check-comment-citations.test.mjs scripts/check-docs-snippet-motion.test.mjs scripts/registry-target-paths.test.mjs scripts/check-workspace-contracts.test.mjs scripts/check-media-use-copy-parity.test.mjs scripts/check-svg-sanitize-parity.test.mjs scripts/check-media-use-svg-sanitize-generated.test.mjs scripts/check-package-cycles.test.mjs scripts/check-cli-process-ownership.test.mjs scripts/check-large-files.test.mjs scripts/package-subpaths.test.mjs scripts/validate-release-channel.test.mjs scripts/publish-workflow.test.mjs scripts/pr-edit-concurrency.test.mjs scripts/install-workspace-dependencies.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts scripts/catalog-payload-assets.test.ts scripts/host-registry-assets.test.ts scripts/catalog-preview-temp.test.ts scripts/catalog-hosted-files.test.ts scripts/player-cdn-pin.test.ts scripts/studio-runtime-smoke.test.mjs scripts/verify-packed-manifests.test.mjs scripts/lint-skills.test.mjs scripts/creator-editing-recipes.test.mjs packages/gcp-cloud-run/check-dockerfile-workspaces.test.mjs packages/core/scripts/writeGeneratedFile.test.ts scripts/catalog-publication.test.mjs scripts/ci/resolve-workflow-pr.test.mjs scripts/check-catalog-source-pr.test.mjs scripts/generate-registry-items.test.ts scripts/catalog-drift.test.ts scripts/catalog-fetch-mirror.test.ts scripts/catalog-script-inlining.test.ts scripts/catalog-detail.test.ts scripts/generate-catalog-pages.test.ts scripts/verify-catalog-payloads.test.ts scripts/registry-skill-files.test.ts scripts/creator-editing-capabilities.test.mjs scripts/generate-catalog-previews.test.ts scripts/registry-primitive-payloads.test.ts registry/components/pan-stations/pan-stations.test.mjs && vitest run scripts/catalog/ scripts/contrastRatchet.test.ts scripts/generate-catalog-payloads.test.ts",
55+
"test:scripts": "node --import tsx --test scripts/animejs-v4-guidance.test.mjs scripts/check-tracked-artifacts.test.mjs scripts/check-registry-set-delta.test.mjs scripts/check-no-main-deletions.test.mjs scripts/check-pr-captures.test.mjs scripts/check-comment-citations.test.mjs scripts/comment-ratchet.test.mjs scripts/check-docs-snippet-motion.test.mjs scripts/registry-target-paths.test.mjs scripts/check-workspace-contracts.test.mjs scripts/check-media-use-copy-parity.test.mjs scripts/check-svg-sanitize-parity.test.mjs scripts/check-media-use-svg-sanitize-generated.test.mjs scripts/check-package-cycles.test.mjs scripts/check-cli-process-ownership.test.mjs scripts/check-large-files.test.mjs scripts/package-subpaths.test.mjs scripts/validate-release-channel.test.mjs scripts/publish-workflow.test.mjs scripts/pr-edit-concurrency.test.mjs scripts/install-workspace-dependencies.test.mjs scripts/draft-changelog.test.ts scripts/set-version.test.ts scripts/release-prepare.test.ts scripts/cli-options.test.ts scripts/changelog-weekly.test.ts scripts/claude-plugin-compression.test.ts scripts/catalog-payload-assets.test.ts scripts/host-registry-assets.test.ts scripts/catalog-preview-temp.test.ts scripts/catalog-hosted-files.test.ts scripts/player-cdn-pin.test.ts scripts/studio-runtime-smoke.test.mjs scripts/verify-packed-manifests.test.mjs scripts/lint-skills.test.mjs scripts/creator-editing-recipes.test.mjs packages/gcp-cloud-run/check-dockerfile-workspaces.test.mjs packages/core/scripts/writeGeneratedFile.test.ts scripts/catalog-publication.test.mjs scripts/ci/resolve-workflow-pr.test.mjs scripts/check-catalog-source-pr.test.mjs scripts/generate-registry-items.test.ts scripts/catalog-drift.test.ts scripts/catalog-fetch-mirror.test.ts scripts/catalog-script-inlining.test.ts scripts/catalog-detail.test.ts scripts/generate-catalog-pages.test.ts scripts/verify-catalog-payloads.test.ts scripts/registry-skill-files.test.ts scripts/creator-editing-capabilities.test.mjs scripts/generate-catalog-previews.test.ts scripts/registry-primitive-payloads.test.ts registry/components/pan-stations/pan-stations.test.mjs && vitest run scripts/catalog/ scripts/contrastRatchet.test.ts scripts/generate-catalog-payloads.test.ts",
5656
"typecheck:scripts": "tsc --noEmit -p scripts/tsconfig.json",
5757
"test:skills": "node --test 'skills/**/*.test.mjs' 'packages/cli/src/media-use/**/*.test.mjs'",
5858
"generate:previews": "tsx scripts/generate-template-previews.ts",

‎scripts/comment-ratchet.mjs‎

Lines changed: 149 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,149 @@
1+
#!/usr/bin/env node
2+
// fallow-ignore-file complexity
3+
// A changed source file's comment share may not rise versus its own copy at the merge-base, and it
4+
// may not gain a comment block over 12 lines. A new file may not open above its package's share.
5+
// Rules and reasons: CONTRIBUTING.md, "Comments". Line counting is owned by the citation checker.
6+
import { execFileSync } from "node:child_process";
7+
import { readFileSync } from "node:fs";
8+
import { extname } from "node:path";
9+
import { commentBlocks } from "./check-comment-citations.mjs";
10+
11+
const MAX_BLOCK_LINES = 12;
12+
13+
// Product source only: a test's table of cases reads as comment and is the test's content.
14+
export const isSource = (path) =>
15+
/^packages\/[^/]+\/src\/.+\.tsx?$/.test(path) && !/\.(d|test|spec)\.tsx?$/.test(path);
16+
17+
const packageOf = (path) => path.split("/").slice(0, 2).join("/");
18+
19+
const git = (args) => execFileSync("git", args, { encoding: "utf8", maxBuffer: 256e6 });
20+
21+
// `-M` names both paths of a rename, so a renamed file keeps its own history instead of being
22+
// graded as new.
23+
export function changedPaths(nameStatus) {
24+
return nameStatus
25+
.split("\n")
26+
.filter(Boolean)
27+
.map((line) => {
28+
const [status, ...paths] = line.split("\t");
29+
const [from, to] = /^[RC]\d*$/.test(status) ? paths : [paths[0], paths[0]];
30+
return { status: status[0], from, to };
31+
})
32+
.filter((entry) => entry.from !== undefined && entry.to !== undefined);
33+
}
34+
35+
// Volume counts lines carrying prose, not a block's span: deleting the code between two blocks
36+
// merges their spans, which would read as comments appearing from nowhere.
37+
export function measure(source, ext) {
38+
const blocks = commentBlocks(source, ext);
39+
const span = (block) => block.end - block.start + 1;
40+
return {
41+
lines: source.split("\n").length,
42+
comment: blocks.reduce((total, block) => total + block.textLines, 0),
43+
long: blocks.filter((block) => !block.marked && span(block) > MAX_BLOCK_LINES).length,
44+
};
45+
}
46+
47+
const share = (entry) => (entry.lines === 0 ? 0 : entry.comment / entry.lines);
48+
const percent = (entry) => `${(share(entry) * 100).toFixed(1)}% (${entry.comment}/${entry.lines})`;
49+
50+
const WALL_ADVICE =
51+
` Cut it to the why and the invariant, or mark its first line "comment-length: <reason>"\n` +
52+
` if it genuinely must stay (a licence, a diagram, a protocol table).`;
53+
54+
// `base` is the file at the merge-base, or null when new. Share fails only when comment lines
55+
// were added too, since deleting code lifts the share of comments nobody touched.
56+
export function judge(file, head, base, packageShare) {
57+
const problems = [];
58+
if (base === null) {
59+
if (share(head) > packageShare) {
60+
problems.push(
61+
`NEW file over its package's comment share: ${file} is ${percent(head)}, the package is ` +
62+
`${(packageShare * 100).toFixed(1)}%\n` +
63+
` A new file starts at or under what its package already carries. Cut the prose, or\n` +
64+
` move the explanation into a name or a type.`,
65+
);
66+
}
67+
if (head.long > 0) {
68+
problems.push(
69+
`NEW comment block over ${MAX_BLOCK_LINES} lines: ${file} has ${head.long}\n${WALL_ADVICE}`,
70+
);
71+
}
72+
return problems;
73+
}
74+
if (share(head) > share(base) && head.comment > base.comment) {
75+
problems.push(
76+
`RAISED comment share: ${file} is ${percent(head)}, at the base ${percent(base)}\n` +
77+
` ${head.comment - base.comment} comment line(s) added. Cut them, or move the explanation\n` +
78+
` into a name or a type. A file's comment share may only go down.`,
79+
);
80+
}
81+
if (head.long > base.long) {
82+
problems.push(
83+
`NEW comment block over ${MAX_BLOCK_LINES} lines: ${file} has ${head.long}, the base has ${base.long}\n${WALL_ADVICE}`,
84+
);
85+
}
86+
return problems;
87+
}
88+
89+
// Untrimmed: dropping the trailing newline shortens the base by a line and raises its share.
90+
function atBase(ref, file) {
91+
try {
92+
return execFileSync("git", ["show", `${ref}:${file}`], { encoding: "utf8", stdio: "pipe" });
93+
} catch {
94+
return null;
95+
}
96+
}
97+
98+
// The bar for a new file: its package's share over files that existed at the base, so new files
99+
// never set their own bar. A package that is new as a whole is held to every package's share.
100+
function packageTotal(dir, isNew) {
101+
return git(["ls-files", dir])
102+
.split("\n")
103+
.filter((file) => isSource(file) && !isNew.has(file))
104+
.reduce(
105+
(sum, file) => {
106+
const entry = measure(readFileSync(file, "utf8"), extname(file));
107+
return { lines: sum.lines + entry.lines, comment: sum.comment + entry.comment };
108+
},
109+
{ lines: 0, comment: 0 },
110+
);
111+
}
112+
113+
function packageShare(pkg, isNew) {
114+
const own = packageTotal(`${pkg}/src`, isNew);
115+
return share(own.lines > 0 ? own : packageTotal("packages", isNew));
116+
}
117+
118+
if (process.argv[1] && import.meta.url === `file://${process.argv[1]}`) {
119+
const ref = git(["merge-base", process.env.COMMENT_CHECK_BASE ?? "origin/main", "HEAD"]).trim();
120+
// Against the working tree, where contents are read from; in CI that is HEAD.
121+
const changed = changedPaths(git(["diff", "--name-status", "-M", ref, "--", "packages"]))
122+
.filter(({ status, to }) => status !== "D" && isSource(to))
123+
.sort((a, b) => a.to.localeCompare(b.to));
124+
125+
const graded = changed.map(({ from, to }) => ({ to, onBase: atBase(ref, from) }));
126+
const isNew = new Set(graded.filter(({ onBase }) => onBase === null).map(({ to }) => to));
127+
const shares = new Map();
128+
const problems = [];
129+
for (const { to, onBase } of graded) {
130+
const head = measure(readFileSync(to, "utf8"), extname(to));
131+
const pkg = packageOf(to);
132+
if (onBase === null && !shares.has(pkg)) shares.set(pkg, packageShare(pkg, isNew));
133+
const base = onBase === null ? null : measure(onBase, extname(to));
134+
problems.push(...judge(to, head, base, shares.get(pkg) ?? 0));
135+
}
136+
137+
const short = git(["rev-parse", "--short", ref]).trim();
138+
if (problems.length === 0) {
139+
console.log(
140+
`comment-ratchet: ${changed.length} changed source file(s) at or under their share at the merge-base ${short}.`,
141+
);
142+
process.exit(0);
143+
}
144+
for (const problem of problems) console.error(problem);
145+
console.error(
146+
`\n${problems.length} problem(s) across ${changed.length} changed source file(s), against the merge-base ${short}.`,
147+
);
148+
process.exit(1);
149+
}

0 commit comments

Comments
 (0)