diff --git a/docs/technical/cli.md b/docs/technical/cli.md index fb3240c..ae76e3c 100644 --- a/docs/technical/cli.md +++ b/docs/technical/cli.md @@ -54,7 +54,7 @@ src/ generate.ts # Documentation generation and watch mode validate.ts # Coverage validation with JSON output diff.ts # DocIR diff against a Git ref and breaking-change gate - adr.ts # Architecture Decision Records (114 lines) + adr.ts # Architecture Decision Records (124 lines) reporters/ index.ts # CI output formatters (82 lines) ``` @@ -433,7 +433,7 @@ Comparing documentation against HEAD~1... ### 4.5 `docgen adr [title]` -**Source:** `src/commands/adr.ts` (114 lines) +**Source:** `src/commands/adr.ts` (124 lines) **Signature:** @@ -474,7 +474,7 @@ Creates a new Architecture Decision Record file. **Validation:** If `title` is not provided, prints an error and exits with code 1. -**ID Assignment:** Reads all files in the ADR directory matching the pattern `/^ADR-\d+/`, counts them, and assigns the next sequential number. IDs are zero-padded to three digits: `ADR-001`, `ADR-002`, etc. +**ID Assignment:** Reads the names of the files in the ADR directory that match `/^ADR-(\d+)/` and assigns one more than the highest number found (`nextAdrNumber()`), so a gap or a deleted ADR never causes a number to be reused. Files that are not numbered ADRs, such as a `README.md` or `ADR-template.md`, are ignored. With none, the first ADR is `ADR-001`. IDs are zero-padded to three digits (`ADR-001`, `ADR-002`, ...) and grow past that without truncation (`ADR-1000`). **File Naming:** The file name is constructed as: diff --git a/docs/technical/parser-typescript.md b/docs/technical/parser-typescript.md index c55fafe..b486388 100644 --- a/docs/technical/parser-typescript.md +++ b/docs/technical/parser-typescript.md @@ -670,16 +670,18 @@ text.replace(/import\([^)]+\)\./g, "") --- -#### `extractParameters(params: ParameterDeclaration[]): ParamNode[]` +#### `extractParameters(params: ParameterDeclaration[], docNode: Node): ParamNode[]` Maps an array of ts-morph `ParameterDeclaration` nodes to DocIR `ParamNode` -objects. +objects. `docNode` is the node that carries the JSDoc block: the function, +method, constructor or accessor itself, or the variable statement for an arrow +function assigned to a `const`. ```typescript { name: p.getName(), type: this.buildTypeRef(p.getType()), - description: this.getParamDescription(p), + description: this.getParamDescription(p, docNode), isOptional: p.isOptional(), isRest: p.isRestParameter(), defaultValue: p.getInitializer()?.getText(), @@ -688,17 +690,18 @@ objects. --- -#### `getParamDescription(param: ParameterDeclaration): string` +#### `getParamDescription(param: ParameterDeclaration, docNode: Node): string` -Walks up to the parameter's parent function/method, extracts all `@param` -tags, and finds the one whose `value` starts with the parameter name. Then -strips the parameter name and any leading `- ` separator from the value. +Looks through the JSDoc blocks on `docNode` for the `@param` tag whose name is +exactly the parameter's name (so `id` never picks up the text written for +`idx`). The text is the tag's comment: the optional `{type}`, the name and a +leading `-` are not part of it, and line breaks inside a multi-line description +become single spaces. -```typescript -paramTag.value.replace(new RegExp(`^${param.getName()}\\s*-?\\s*`), "") -``` - -Returns `""` if no matching `@param` tag is found. +Returns `""` if no matching `@param` tag exists. Both `@param a - text` and +`@param a text` are accepted. Before this was corrected, `@param` text never +reached `ParamNode.description`, so documented parameters were reported as +undocumented by `docgen validate`. --- diff --git a/packages/cli/src/commands/adr.test.ts b/packages/cli/src/commands/adr.test.ts index c1416a7..707e46d 100644 --- a/packages/cli/src/commands/adr.test.ts +++ b/packages/cli/src/commands/adr.test.ts @@ -126,6 +126,71 @@ describe("adrCommand", () => { }); }); - // Known gap found by the audit. `todo` keeps it visible without a red run. - it.todo("numbers a new ADR after the highest existing number, not after the file count (ADR-001 and ADR-003 present creates a second ADR-003)"); + // The audit found that numbering used the file count, so gaps produced duplicate numbers. + describe("numbering with gaps and other files", () => { + const seed = (...names: string[]) => { + fs.mkdirSync(decisions(), { recursive: true }); + for (const name of names) fs.writeFileSync(path.join(decisions(), name), `# ${name}\n`); + }; + + it("numbers after the highest existing number, not after the file count", async () => { + seed("ADR-001-first.md", "ADR-003-third.md"); + + await adrCommand("new", "Fourth", { status: "proposed" }); + + expect(fs.readdirSync(decisions()).sort()).toEqual([ + "ADR-001-first.md", + "ADR-003-third.md", + "ADR-004-fourth.md", + ]); + }); + + it("does not reuse a number after an earlier ADR was deleted", async () => { + await adrCommand("new", "One", { status: "proposed" }); + await adrCommand("new", "Two", { status: "proposed" }); + fs.rmSync(path.join(decisions(), fs.readdirSync(decisions()).sort()[0])); + + await adrCommand("new", "Three", { status: "proposed" }); + + expect(fs.readdirSync(decisions()).sort()).toEqual([ + expect.stringMatching(/^ADR-002-two/), + expect.stringMatching(/^ADR-003-three/), + ]); + }); + + it("keeps counting past three digits instead of truncating or repeating", async () => { + seed("ADR-999-nine-nine-nine.md", "ADR-1000-one-thousand.md"); + + await adrCommand("new", "Next", { status: "proposed" }); + + expect(fs.readdirSync(decisions())).toContain("ADR-1001-next.md"); + }); + + it("ignores files that are not numbered ADRs when choosing the number", async () => { + seed("README.md", "ADR-template.md", "notes-2024.md", "ADR-002-real.md"); + + await adrCommand("new", "After real", { status: "proposed" }); + + expect(fs.readdirSync(decisions())).toContain("ADR-003-after-real.md"); + expect(fs.readdirSync(decisions())).toHaveLength(5); + }); + + it("starts at ADR-001 when the folder holds no numbered ADRs", async () => { + seed("README.md"); + + await adrCommand("new", "First", { status: "proposed" }); + + expect(fs.readdirSync(decisions())).toContain("ADR-001-first.md"); + }); + + it("never overwrites an existing file", async () => { + seed("ADR-001-first.md"); + const before = fs.readFileSync(path.join(decisions(), "ADR-001-first.md"), "utf-8"); + + await adrCommand("new", "First", { status: "proposed" }); + + expect(fs.readFileSync(path.join(decisions(), "ADR-001-first.md"), "utf-8")).toBe(before); + expect(fs.readdirSync(decisions())).toContain("ADR-002-first.md"); + }); + }); }); diff --git a/packages/cli/src/commands/adr.ts b/packages/cli/src/commands/adr.ts index 79cb989..1608b81 100644 --- a/packages/cli/src/commands/adr.ts +++ b/packages/cli/src/commands/adr.ts @@ -33,6 +33,23 @@ export async function adrCommand( } } +/** + * Choose the number for a new ADR: one more than the highest `ADR-` file name. + * Counting files would reuse a number after a deletion or a gap, and files that + * are not numbered ADRs (a README, a template) must not affect the result. + * + * @param fileNames - Names of the files in the ADR directory. + * @returns The number to give the next ADR; 1 when there are no numbered ADRs. + */ +export function nextAdrNumber(fileNames: string[]): number { + let highest = 0; + for (const name of fileNames) { + const match = /^ADR-(\d+)/.exec(name); + if (match) highest = Math.max(highest, Number(match[1])); + } + return highest + 1; +} + function createAdr( adrDir: string, title: string | undefined, @@ -47,10 +64,7 @@ function createAdr( fs.mkdirSync(adrDir, { recursive: true }); } - // Determine next ADR number - const existing = fs.readdirSync(adrDir).filter((f) => f.match(/^ADR-\d+/)); - const nextNum = existing.length + 1; - const id = `ADR-${String(nextNum).padStart(3, "0")}`; + const id = `ADR-${String(nextAdrNumber(fs.readdirSync(adrDir))).padStart(3, "0")}`; const fileName = `${id}-${title.toLowerCase().replace(/[^a-z0-9]+/g, "-")}.md`; const filePath = path.join(adrDir, fileName); diff --git a/packages/parser-typescript/src/index.ts b/packages/parser-typescript/src/index.ts index e4270ec..fb30bb6 100644 --- a/packages/parser-typescript/src/index.ts +++ b/packages/parser-typescript/src/index.ts @@ -281,7 +281,7 @@ export class TypeScriptParser implements ParserPlugin { isAbstract: false, signature: method.getText(), description: this.extractDescription(method), - parameters: this.extractParameters(method.getParameters()), + parameters: this.extractParameters(method.getParameters(), method), returnType: this.buildTypeRef(method.getReturnType()), throws: [], deprecated: null, @@ -440,7 +440,7 @@ export class TypeScriptParser implements ParserPlugin { isAbstract: false, signature: this.buildFunctionSignature(func), description: this.extractDescription(func), - parameters: this.extractParameters(func.getParameters()), + parameters: this.extractParameters(func.getParameters(), func), returnType: this.buildTypeRef(func.getReturnType()), throws: this.extractThrows(func), deprecated: null, @@ -498,7 +498,7 @@ export class TypeScriptParser implements ParserPlugin { isAbstract: false, signature: this.buildVariableFunctionSignature(declaration, initializer), description: this.extractDescription(docNode), - parameters: this.extractParameters(initializer.getParameters()), + parameters: this.extractParameters(initializer.getParameters(), docNode), returnType: this.buildTypeRef(initializer.getReturnType()), throws: this.extractThrows(docNode), deprecated: this.extractDeprecation(docNode), @@ -715,7 +715,7 @@ export class TypeScriptParser implements ParserPlugin { isAbstract: method.isAbstract(), signature: this.buildMethodSignature(method), description: this.extractDescription(method), - parameters: this.extractParameters(method.getParameters()), + parameters: this.extractParameters(method.getParameters(), method), returnType: this.buildTypeRef(method.getReturnType()), throws: this.extractThrows(method), deprecated: this.extractDeprecation(method), @@ -761,7 +761,7 @@ export class TypeScriptParser implements ParserPlugin { .map((p) => p.getText()) .join(", ")})`, description: this.extractDescription(ctor), - parameters: this.extractParameters(ctor.getParameters()), + parameters: this.extractParameters(ctor.getParameters(), ctor), returnType: null, throws: [], deprecated: null, @@ -787,7 +787,8 @@ export class TypeScriptParser implements ParserPlugin { parameters: kind === "setter" ? this.extractParameters( - (accessor as SetAccessorDeclaration).getParameters() + (accessor as SetAccessorDeclaration).getParameters(), + accessor ) : [], returnType: @@ -904,32 +905,38 @@ export class TypeScriptParser implements ParserPlugin { return text.replace(/import\([^)]+\)\./g, ""); } - private extractParameters(params: ParameterDeclaration[]): ParamNode[] { + /** + * Build parameter nodes for a callable. + * @param params - The declared parameters, in order. + * @param docNode - The node that carries the JSDoc block: the function itself, or the variable + * statement for an arrow function assigned to a const. + */ + private extractParameters(params: ParameterDeclaration[], docNode: Node): ParamNode[] { return params.map((p) => ({ name: p.getName(), type: this.buildTypeRef(p.getType()), - description: this.getParamDescription(p), + description: this.getParamDescription(p, docNode), isOptional: p.isOptional(), isRest: p.isRestParameter(), defaultValue: p.getInitializer()?.getText(), })); } - private getParamDescription(param: ParameterDeclaration): string { - // Walk up to the function/method and find matching @param tag - const parent = param.getParent(); - if (!parent) return ""; - - const tags = this.extractTags(parent); - const paramTag = tags.find( - (t) => t.tag === "param" && t.description.startsWith(param.getName()) - ); - - if (paramTag) { - // Strip the param name from the value - return paramTag.description.replace(new RegExp(`^${param.getName()}\\s*-?\\s*`), ""); + /** + * Find the `@param` text for one parameter. A tag's name is matched exactly (so `id` never picks up + * `idx`'s text), and the optional `{type}`, the name and a leading hyphen are not part of the text. + */ + private getParamDescription(param: ParameterDeclaration, docNode: Node): string { + const name = param.getName(); + for (const doc of this.getJsDocs(docNode)) { + for (const tag of doc.getTags()) { + if (!Node.isJSDocParameterTag(tag) || tag.getName() !== name) continue; + return (tag.getCommentText() ?? "") + .replace(/^\s*-\s*/, "") + .replace(/\s*\n\s*/g, " ") + .trim(); + } } - return ""; } diff --git a/packages/parser-typescript/src/param-docs.test.ts b/packages/parser-typescript/src/param-docs.test.ts new file mode 100644 index 0000000..e54aa16 --- /dev/null +++ b/packages/parser-typescript/src/param-docs.test.ts @@ -0,0 +1,115 @@ +import * as fs from "fs"; +import * as path from "path"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("@docgen/core", async () => await import("../../core/src")); + +// Each parse builds a ts-morph project; the first one in a cold process is slow. +vi.setConfig({ testTimeout: 30_000 }); + +// AUDIT-001 follow-up: `@param name - text` tags did not reach ParamNode.description, so RepoScribe's own +// validation listed every documented parameter as undocumented. These tests pin the intended behaviour. + +const langConfig = { + name: "typescript", + source: "src", + include: ["**/*.ts"], + exclude: [], + parser: "@docgen/parser-typescript", + options: {}, +}; + +describe("TypeScriptParser @param descriptions", () => { + let dir: string; + + beforeEach(() => { + dir = fs.mkdtempSync(path.join(process.cwd(), ".tmp-param-docs-")); + fs.mkdirSync(path.join(dir, "src"), { recursive: true }); + }); + + afterEach(() => { + fs.rmSync(dir, { recursive: true, force: true }); + }); + + async function parameters(source: string, memberName: string): Promise> { + const file = path.join(dir, "src", "sample.ts"); + fs.writeFileSync(file, source); + const { TypeScriptParser } = await import("./index"); + const docir = await new TypeScriptParser().parse([file], langConfig); + const members = docir.modules.flatMap((module) => module.members); + const member = members.find((candidate) => candidate.name === memberName); + if (!member) throw new Error(`member ${memberName} not found in ${members.map((m) => m.name).join(", ")}`); + return Object.fromEntries(member.parameters.map((parameter) => [parameter.name, parameter.description])); + } + + it("reads a description written with a hyphen after the name", async () => { + const result = await parameters( + ["/**", " * Add.", " * @param a - The first number.", " * @param b - The second number.", " */", + "export function add(a: number, b: number): number { return a + b; }"].join("\n"), + "add" + ); + expect(result).toEqual({ a: "The first number.", b: "The second number." }); + }); + + it("reads a description written without a hyphen", async () => { + const result = await parameters( + ["/**", " * Add.", " * @param a The first number.", " */", "export function add(a: number): number { return a; }"].join("\n"), + "add" + ); + expect(result).toEqual({ a: "The first number." }); + }); + + it("ignores a {type} annotation in the tag", async () => { + const result = await parameters( + ["/**", " * Add.", " * @param {number} a - The first number.", " */", "export function add(a: number): number { return a; }"].join("\n"), + "add" + ); + expect(result).toEqual({ a: "The first number." }); + }); + + it("gives each parameter its own text when names share a prefix", async () => { + const result = await parameters( + ["/**", " * Find.", " * @param idx - The index.", " * @param id - The identifier.", " */", + "export function find(id: string, idx: number): string { return id + idx; }"].join("\n"), + "find" + ); + expect(result).toEqual({ id: "The identifier.", idx: "The index." }); + }); + + it("leaves an undocumented parameter empty instead of borrowing another tag", async () => { + const result = await parameters( + ["/**", " * Add.", " * @param a - The first number.", " */", + "export function add(a: number, b: number): number { return a + b; }"].join("\n"), + "add" + ); + expect(result).toEqual({ a: "The first number.", b: "" }); + }); + + it("keeps a multi-line description together", async () => { + const result = await parameters( + ["/**", " * Add.", " * @param a - The first number,", " * which may be negative.", " */", + "export function add(a: number): number { return a; }"].join("\n"), + "add" + ); + expect(result.a).toContain("The first number,"); + expect(result.a).toContain("which may be negative."); + }); + + it("reads descriptions on arrow functions assigned to a const", async () => { + const result = await parameters( + ["/**", " * Double.", " * @param n - The number to double.", " */", + "export const double = (n: number): number => n * 2;"].join("\n"), + "double" + ); + expect(result).toEqual({ n: "The number to double." }); + }); + + it("reads descriptions on class methods", async () => { + const result = await parameters( + ["export class Calculator {", " /**", " * Add.", " * @param a - Left side.", " * @param b - Right side.", " */", + " add(a: number, b: number): number { return a + b; }", "}"].join("\n"), + "add" + ); + expect(result).toEqual({ a: "Left side.", b: "Right side." }); + }); +});