-
Notifications
You must be signed in to change notification settings - Fork 0
Add custom ESLint rule scaffolding for Rig #125
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
9b5a37d
f54c4de
f704cc4
52922ee
27e4363
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| import noObjectLiteralRecord from "./rules/no-object-literal-record.js"; | ||
|
|
||
| export default { | ||
| meta: { | ||
| name: "rig", | ||
| }, | ||
| rules: { | ||
| "no-object-literal-record": noObjectLiteralRecord, | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,166 @@ | ||
| #!/usr/bin/env node | ||
|
|
||
| import { readFile, readdir, writeFile } from "node:fs/promises"; | ||
| import { extname, resolve } from "node:path"; | ||
| import { pathToFileURL } from "node:url"; | ||
|
|
||
| const methods = new Set(["record", "nonEmptyObject"]); | ||
| const ignoredDirectories = new Set([".git", "node_modules"]); | ||
|
|
||
| function tokenize(source) { | ||
| const tokens = []; | ||
| let index = 0; | ||
|
|
||
| while (index < source.length) { | ||
| const start = index; | ||
| const char = source[index]; | ||
| const next = source[index + 1]; | ||
|
|
||
| if (/\s/.test(char)) { | ||
| index += 1; | ||
| } else if (char === "/" && next === "/") { | ||
| index = source.indexOf("\n", index + 2); | ||
| if (index === -1) break; | ||
| } else if (char === "/" && next === "*") { | ||
| index = source.indexOf("*/", index + 2); | ||
| index = index === -1 ? source.length : index + 2; | ||
| } else if (char === "'" || char === "\"" || char === "`") { | ||
| const quote = char; | ||
| index += 1; | ||
| while (index < source.length) { | ||
| if (source[index] === "\\") { | ||
| index += 2; | ||
| } else if (source[index] === quote) { | ||
| index += 1; | ||
| break; | ||
| } else { | ||
| index += 1; | ||
| } | ||
| } | ||
| } else if (/[A-Za-z_$]/.test(char)) { | ||
| index += 1; | ||
| while (index < source.length && /[\w$]/.test(source[index])) index += 1; | ||
| tokens.push({ value: source.slice(start, index), start, end: index }); | ||
| } else { | ||
| index += 1; | ||
| tokens.push({ value: char, start, end: index }); | ||
| } | ||
| } | ||
|
|
||
| return tokens; | ||
| } | ||
|
|
||
| function closingBrace(tokens, openingIndex) { | ||
| let depth = 0; | ||
| for (let index = openingIndex; index < tokens.length; index += 1) { | ||
| if (tokens[index].value === "{") depth += 1; | ||
| if (tokens[index].value === "}") depth -= 1; | ||
| if (depth === 0) return index; | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| export function lintSource(source) { | ||
| const tokens = tokenize(source); | ||
| const problems = []; | ||
|
|
||
| for (let index = 0; index <= tokens.length - 5; index += 1) { | ||
| const [schema, dot, method, openCall] = tokens.slice(index, index + 4); | ||
| if ( | ||
| tokens[index - 1]?.value === "." | ||
| || schema.value !== "s" | ||
| || dot.value !== "." | ||
| || !methods.has(method.value) | ||
| || openCall.value !== "(" | ||
| ) { | ||
| continue; | ||
| } | ||
|
|
||
| let objectIndex = index + 4; | ||
| while (tokens[objectIndex]?.value === "(") objectIndex += 1; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] The parenthesis-skip loop ( 💡 Missing test caseit('handles nested parens with comment between braces', () => {
const src = 's.record(/* x */({ count: s.number }))';
expect(lintSource(src)).toHaveLength(1);
});Adding this test will surface whether the wrapper-check logic is robust. |
||
| const object = tokens[objectIndex]; | ||
| if (object?.value !== "{") continue; | ||
|
|
||
| const closingIndex = closingBrace(tokens, objectIndex); | ||
| const wrapperCount = objectIndex - (index + 4); | ||
| const wrappersClose = Array.from( | ||
| { length: wrapperCount }, | ||
| (_, offset) => tokens[(closingIndex ?? tokens.length) + offset + 1]?.value, | ||
| ).every((value) => value === ")"); | ||
| if (closingIndex !== undefined && wrappersClose) { | ||
| problems.push({ | ||
| start: object.start, | ||
| end: tokens[closingIndex].end, | ||
| message: `Wrap object-valued record fields with s.object(...).`, | ||
| }); | ||
| } | ||
| } | ||
|
|
||
| return problems; | ||
| } | ||
|
|
||
| export function fixSource(source, problems = lintSource(source)) { | ||
| let fixed = source; | ||
| const edits = problems | ||
| .flatMap(({ start, end }) => [ | ||
| { index: start, text: "s.object(" }, | ||
| { index: end, text: ")" }, | ||
| ]) | ||
| .sort((left, right) => right.index - left.index); | ||
| for (const edit of edits) { | ||
| fixed = `${fixed.slice(0, edit.index)}${edit.text}${fixed.slice(edit.index)}`; | ||
| } | ||
| return fixed; | ||
| } | ||
|
|
||
| async function sourceFiles(paths) { | ||
| const files = []; | ||
| for (const path of paths) { | ||
| const entries = await readdir(path, { withFileTypes: true }).catch(() => undefined); | ||
| if (!entries) { | ||
| if (extname(path) === ".ts") files.push(path); | ||
| continue; | ||
| } | ||
| for (const entry of entries) { | ||
| if (entry.isDirectory() && ignoredDirectories.has(entry.name)) continue; | ||
| const child = resolve(path, entry.name); | ||
| if (entry.isDirectory()) files.push(...await sourceFiles([child])); | ||
| else if (extname(entry.name) === ".ts") files.push(child); | ||
| } | ||
| } | ||
| return files; | ||
| } | ||
|
|
||
| async function main(argv) { | ||
| const fix = argv.includes("--fix"); | ||
| const paths = argv.filter((arg) => arg !== "--fix").map((path) => resolve(path)); | ||
| if (paths.length === 0) { | ||
| throw new Error("Usage: node skills/rig/eslint/lint.js [--fix] <file-or-directory> [...]"); | ||
| } | ||
|
|
||
| let failures = 0; | ||
| for (const file of await sourceFiles(paths)) { | ||
| const source = await readFile(file, "utf8"); | ||
| const problems = lintSource(source); | ||
| if (problems.length === 0) continue; | ||
| if (fix) { | ||
| await writeFile(file, fixSource(source, problems)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/tdd] 💡 SuggestionEither let the error propagate naturally (already handled by try {
await writeFile(file, fixSource(source, problems));
} catch (err) {
console.error(`Failed to write ${file}: ${err.message}`);
failures += 1;
}Also add a test for the |
||
| continue; | ||
| } | ||
| failures += problems.length; | ||
| for (const problem of problems) { | ||
| const line = source.slice(0, problem.start).split("\n").length; | ||
| console.error(`${file}:${line}: ${problem.message}`); | ||
| } | ||
| } | ||
|
|
||
| if (failures > 0) process.exitCode = 1; | ||
| } | ||
|
|
||
| const isMain = process.argv[1] && pathToFileURL(resolve(process.argv[1])).href === import.meta.url; | ||
| if (isMain) { | ||
| main(process.argv.slice(2)).catch((error) => { | ||
| console.error(error.message); | ||
| process.exitCode = 1; | ||
| }); | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/codebase-design] The test at 💡 Suggestion: share fixtures across both implementationsExtract valid/invalid case arrays and run them against both const INVALID = [
'const x = s.record({ count: s.number });',
'const x = s.nonEmptyObject({ count: s.number });',
];
it.each(INVALID)('lintSource flags %s', (src) => {
expect(lintSource(src)).toHaveLength(1);
});
// same list fed to RuleTester |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,48 @@ | ||
| export default { | ||
| meta: { | ||
| type: "problem", | ||
| docs: { | ||
| description: "Require object-valued record schemas to use s.object", | ||
| }, | ||
| fixable: "code", | ||
| schema: [], | ||
| messages: { | ||
| wrapObject: "Wrap object-valued record fields with {{schema}}.object(...).", | ||
| }, | ||
| }, | ||
| create(context) { | ||
| const sourceCode = context.sourceCode; | ||
|
|
||
| return { | ||
| CallExpression(node) { | ||
| const { callee } = node; | ||
| if ( | ||
| callee.type !== "MemberExpression" | ||
| || callee.computed | ||
| || callee.object.type !== "Identifier" | ||
| || callee.object.name !== "s" | ||
| || callee.property.type !== "Identifier" | ||
| || !["record", "nonEmptyObject"].includes(callee.property.name) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| const value = node.arguments[0]; | ||
| if (!value || value.type !== "ObjectExpression") { | ||
| return; | ||
| } | ||
|
|
||
| context.report({ | ||
| node: value, | ||
| messageId: "wrapObject", | ||
| data: { | ||
| schema: callee.object.name, | ||
| }, | ||
| fix(fixer) { | ||
| return fixer.replaceText(value, `${callee.object.name}.object(${sourceCode.getText(value)})`); | ||
| }, | ||
| }); | ||
| }, | ||
| }; | ||
| }, | ||
| }; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| # Linting Rig programs | ||
|
|
||
| Rig includes a dependency-free linter in the skill folder and exports an ESLint plugin from `rig/eslint`. | ||
|
|
||
| Run the linter after generating or changing a Rig program: | ||
|
|
||
| ```bash | ||
| node skills/rig/eslint/lint.js path/to/program.ts | ||
| ``` | ||
|
|
||
| Use `--fix` to apply safe fixes: | ||
|
|
||
| ```bash | ||
| node skills/rig/eslint/lint.js --fix path/to/program.ts | ||
| ``` | ||
|
|
||
| ## Rules | ||
|
|
||
| ### `rig/no-object-literal-record` | ||
|
|
||
| An `s.record` or `s.nonEmptyObject` value must be a schema. Wrap object fields with `s.object`: | ||
|
|
||
| ```ts | ||
| // Invalid | ||
| s.record({ status: s.string }) | ||
|
|
||
| // Valid | ||
| s.record(s.object({ status: s.string })) | ||
| ``` | ||
|
|
||
| The rule fixes the invalid form automatically. | ||
|
|
||
| ## Adding rules | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [/codebase-design] The scaffolding instructions say to add the equivalent check to 💡 SuggestionStrengthen the last bullet:
This makes the dual-implementation expectation explicit. |
||
|
|
||
| Put rule implementations in `skills/rig/eslint/rules/`, export them from `skills/rig/eslint/index.js`, add the equivalent skill-local check to `skills/rig/eslint/lint.js`, and cover both in `src/eslint-rules.test.js`. | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,60 @@ | ||
| import { describe, expect, it } from "vitest"; | ||
| import { fixSource, lintSource } from "../skills/rig/eslint/lint.js"; | ||
| import rule from "../skills/rig/eslint/rules/no-object-literal-record.js"; | ||
|
|
||
| describe("no-object-literal-record", () => { | ||
| it.each([ | ||
| "const output = s.record(s.object({ count: s.number }));", | ||
| "const output = s.nonEmptyObject(s.object({ count: s.number }));", | ||
| "const output = s.record(s.string);", | ||
| "const output = other.record({ count: s.number });", | ||
| "const output = config.s.record({ count: s.number });", | ||
| "const text = 's.record({ count: s.number })';", | ||
| ])("accepts %s", (source) => { | ||
| expect(lintSource(source)).toEqual([]); | ||
| }); | ||
|
|
||
| it.each([ | ||
| [ | ||
| "const output = s.record({ count: s.number });", | ||
| "const output = s.record(s.object({ count: s.number }));", | ||
| ], | ||
| [ | ||
| "const output = s.nonEmptyObject(/* value */ { count: s.number });", | ||
| "const output = s.nonEmptyObject(/* value */ s.object({ count: s.number }));", | ||
| ], | ||
| [ | ||
| "const output = s.record(({ count: s.number }));", | ||
| "const output = s.record((s.object({ count: s.number })));", | ||
| ], | ||
| ])("fixes %s", (source, expected) => { | ||
| const problems = lintSource(source); | ||
| expect(problems).toHaveLength(1); | ||
| expect(fixSource(source, problems)).toBe(expected); | ||
| }); | ||
|
|
||
| it("keeps the ESLint rule aligned", () => { | ||
| const reports = []; | ||
| const object = { type: "ObjectExpression" }; | ||
| const visitor = rule.create({ | ||
| sourceCode: { getText: () => "{ count: s.number }" }, | ||
| report: (problem) => reports.push(problem), | ||
| }); | ||
|
|
||
| visitor.CallExpression({ | ||
| type: "CallExpression", | ||
| callee: { | ||
| type: "MemberExpression", | ||
| computed: false, | ||
| object: { type: "Identifier", name: "s" }, | ||
| property: { type: "Identifier", name: "record" }, | ||
| }, | ||
| arguments: [object], | ||
| }); | ||
|
|
||
| expect(reports).toHaveLength(1); | ||
| expect(reports[0].messageId).toBe("wrapObject"); | ||
| expect(reports[0].fix({ replaceText: (_node, text) => text })) | ||
| .toBe("s.object({ count: s.number })"); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[/tdd] Line 22: if a file ends with a
//comment and no trailing newline,source.indexOf(' ', index + 2)returns-1, the loop breaks, and tokens after the comment are silently skipped.This means a file ending in
// comment(no newline) is tokenised differently from the same file with a trailing newline — a silent correctness bug with no test coverage.💡 Suggested fix + test
Test: