From 98841f5b6c79a2314f5bde6d328fe0b6f4e56bea Mon Sep 17 00:00:00 2001 From: Gerrit Birkeland Date: Sat, 26 Mar 2022 16:15:29 -0600 Subject: [PATCH] Fix validation.notDocumentated signature logic Resolves #1895 --- CHANGELOG.md | 2 + src/lib/converter/plugins/CommentPlugin.ts | 13 ++++- src/lib/converter/symbols.ts | 6 +- src/lib/models/reflections/abstract.ts | 26 ++++++++- src/lib/output/plugins/MarkedLinksPlugin.ts | 2 +- src/lib/utils/options/sources/typedoc.ts | 3 +- src/lib/validation/documentation.ts | 61 +++++++++++++++++---- src/test/TestLogger.ts | 32 +++++++++++ src/test/converter/exports/mod.ts | 4 +- src/test/converter2/issues/gh1547.ts | 4 +- src/test/converter2/issues/gh1580.ts | 5 +- src/test/converter2/validation/class.ts | 8 +++ src/test/converter2/validation/function.ts | 7 +++ src/test/validation.test.ts | 55 +++++++++++++++++-- 14 files changed, 192 insertions(+), 36 deletions(-) create mode 100644 src/test/TestLogger.ts create mode 100644 src/test/converter2/validation/class.ts create mode 100644 src/test/converter2/validation/function.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 66b6a9e74..69f853a15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,7 +3,9 @@ ### Bug Fixes - Fixed missing comments on `@enum` style enum members defined in declaration files, #1880. +- Fixed `--validation.notDocumented` warnings for functions/methods, #1895. - Search results will no longer include random items when the search bar is empty, #1881. +- Comments on overloaded constructors will now be detected in the same way that overloaded functions/methods are. ### Thanks! diff --git a/src/lib/converter/plugins/CommentPlugin.ts b/src/lib/converter/plugins/CommentPlugin.ts index 7609c42d0..039c9f667 100644 --- a/src/lib/converter/plugins/CommentPlugin.ts +++ b/src/lib/converter/plugins/CommentPlugin.ts @@ -175,7 +175,11 @@ export class CommentPlugin extends ConverterComponent { reflection: Reflection, node?: ts.Node ) { - if (reflection.kindOf(ReflectionKind.FunctionOrMethod)) { + if ( + reflection.kindOf( + ReflectionKind.FunctionOrMethod | ReflectionKind.Constructor + ) + ) { // We only want a comment on functions/methods if this is a set of overloaded functions. // In that case, TypeDoc lets you put a comment on the implementation, and will copy it over to // the available signatures so that you can avoid documenting things multiple times. @@ -184,7 +188,9 @@ export class CommentPlugin extends ConverterComponent { let specialOverloadCase = false; if ( node && - (ts.isFunctionDeclaration(node) || ts.isMethodDeclaration(node)) + (ts.isFunctionDeclaration(node) || + ts.isMethodDeclaration(node) || + ts.isConstructorDeclaration(node)) ) { const symbol = node.name && context.checker.getSymbolAtLocation(node.name); @@ -192,7 +198,8 @@ export class CommentPlugin extends ConverterComponent { const declarations = symbol.declarations.filter( (d) => ts.isFunctionDeclaration(d) || - ts.isMethodDeclaration(d) + ts.isMethodDeclaration(d) || + ts.isConstructorDeclaration(d) ); if ( declarations.length > 1 && diff --git a/src/lib/converter/symbols.ts b/src/lib/converter/symbols.ts index 3bd7e2840..19d96f5d1 100644 --- a/src/lib/converter/symbols.ts +++ b/src/lib/converter/symbols.ts @@ -509,10 +509,12 @@ function convertClassOrInterface( reflection ); reflectionContext.addChild(constructMember); - // The symbol is already taken by the class. - context.registerReflection(constructMember, undefined); const ctors = staticType.getConstructSignatures(); + context.registerReflection( + constructMember, + ctors?.[0]?.declaration?.symbol + ); // Modifiers are the same for all constructors if (ctors.length && ctors[0].declaration) { diff --git a/src/lib/models/reflections/abstract.ts b/src/lib/models/reflections/abstract.ts index 51f7b62a7..9a1a76ad7 100644 --- a/src/lib/models/reflections/abstract.ts +++ b/src/lib/models/reflections/abstract.ts @@ -364,7 +364,8 @@ export abstract class Reflection { } /** - * Return the full name of this reflection. + * Return the full name of this reflection. Intended for use in debugging. For log messages + * intended to be displayed to the user for them to fix, prefer {@link getFriendlyFullName} instead. * * The full name contains the name of this reflection and the names of all parent reflections. * @@ -379,6 +380,29 @@ export abstract class Reflection { } } + /** + * Return the full name of this reflection, with signature names dropped if possible without + * introducing ambiguity in the name. + */ + getFriendlyFullName(): string { + if (this.parent && !this.parent.isProject()) { + if ( + this.kindOf( + ReflectionKind.ConstructorSignature | + ReflectionKind.CallSignature | + ReflectionKind.GetSignature | + ReflectionKind.SetSignature + ) + ) { + return this.parent.getFriendlyFullName(); + } + + return this.parent.getFriendlyFullName() + "." + this.name; + } else { + return this.name; + } + } + /** * Set a flag on this reflection. */ diff --git a/src/lib/output/plugins/MarkedLinksPlugin.ts b/src/lib/output/plugins/MarkedLinksPlugin.ts index c54a8cca0..d078068f6 100644 --- a/src/lib/output/plugins/MarkedLinksPlugin.ts +++ b/src/lib/output/plugins/MarkedLinksPlugin.ts @@ -135,7 +135,7 @@ export class MarkedLinksPlugin extends ContextAwareRendererComponent { } } else { const fullName = (this.reflection || - this.project)!.getFullName(); + this.project)!.getFriendlyFullName(); this.warnings.push(`In ${fullName}: ${original}`); return original; } diff --git a/src/lib/utils/options/sources/typedoc.ts b/src/lib/utils/options/sources/typedoc.ts index 039cfe4e9..b3e1b5f88 100644 --- a/src/lib/utils/options/sources/typedoc.ts +++ b/src/lib/utils/options/sources/typedoc.ts @@ -372,8 +372,7 @@ export function addTypeDocOptions(options: Pick) { "Interface", "Property", "Method", - "GetSignature", - "SetSignature", + "Accessor", "TypeAlias", ], }); diff --git a/src/lib/validation/documentation.ts b/src/lib/validation/documentation.ts index 5b657f99c..40c69efb8 100644 --- a/src/lib/validation/documentation.ts +++ b/src/lib/validation/documentation.ts @@ -1,23 +1,66 @@ import * as path from "path"; import * as ts from "typescript"; -import { ProjectReflection, ReflectionKind } from "../models"; +import { + DeclarationReflection, + ProjectReflection, + ReflectionKind, + SignatureReflection, +} from "../models"; import { Logger, normalizePath } from "../utils"; +import { removeFlag } from "../utils/enum"; export function validateDocumentation( project: ProjectReflection, logger: Logger, requiredToBeDocumented: readonly (keyof typeof ReflectionKind)[] ): void { - const kinds = requiredToBeDocumented.reduce( - (prev, cur) => (prev |= ReflectionKind[cur]), + let kinds = requiredToBeDocumented.reduce( + (prev, cur) => prev | ReflectionKind[cur], 0 ); + // Functions, Constructors, and Accessors never have comments directly on them. + // If they are required to be documented, what's really required is that their + // contained signatures have a comment. + if (kinds & ReflectionKind.FunctionOrMethod) { + kinds |= ReflectionKind.CallSignature; + kinds = removeFlag(kinds, ReflectionKind.FunctionOrMethod); + } + if (kinds & ReflectionKind.Constructor) { + kinds |= ReflectionKind.ConstructorSignature; + kinds = removeFlag(kinds, ReflectionKind.Constructor); + } + if (kinds & ReflectionKind.Accessor) { + kinds |= ReflectionKind.GetSignature | ReflectionKind.SetSignature; + kinds = removeFlag(kinds, ReflectionKind.Accessor); + } + for (const ref of project.getReflectionsByKind(kinds)) { - const symbol = project.getSymbolFromReflection(ref); - if (!ref.comment && symbol?.declarations) { - const decl = symbol.declarations[0]; + let symbol = project.getSymbolFromReflection(ref); + let index = 0; + + // Signatures don't have symbols associated with them, so get the parent and then + // maybe also adjust the declaration index that we care about. + if (!symbol && ref.kindOf(ReflectionKind.SomeSignature)) { + symbol = project.getSymbolFromReflection(ref.parent!); + + const parentIndex = ( + ref.parent as DeclarationReflection + ).signatures?.indexOf(ref as SignatureReflection); + if (parentIndex) { + index = parentIndex; + } + } + + const decl = symbol?.declarations?.[index]; + + if (!ref.hasComment() && decl) { const sourceFile = decl.getSourceFile(); + + if (sourceFile.fileName.includes("node_modules")) { + continue; + } + const { line } = ts.getLineAndCharacterOfPosition( sourceFile, decl.getStart() @@ -26,13 +69,9 @@ export function validateDocumentation( path.relative(process.cwd(), sourceFile.fileName) ); - if (file.includes("node_modules")) { - continue; - } - const loc = `${file}:${line + 1}`; logger.warn( - `${ref.name}, defined at ${loc}, does not have any documentation.` + `${ref.getFriendlyFullName()}, defined at ${loc}, does not have any documentation.` ); } } diff --git a/src/test/TestLogger.ts b/src/test/TestLogger.ts new file mode 100644 index 000000000..a94359dbd --- /dev/null +++ b/src/test/TestLogger.ts @@ -0,0 +1,32 @@ +import { Logger, LogLevel } from "../lib/utils"; +import { deepStrictEqual as equal, fail } from "assert"; + +const levelMap: Record = { + [LogLevel.Error]: "error: ", + [LogLevel.Warn]: "warn: ", + [LogLevel.Info]: "info: ", + [LogLevel.Verbose]: "debug: ", +}; + +export class TestLogger extends Logger { + messages: string[] = []; + + expectMessage(message: string) { + const index = this.messages.indexOf(message); + if (index === -1) { + const messages = this.messages.join("\n\t") || "(none logged)"; + fail( + `Expected "${message}" to be logged. The logged messages were:\n\t${messages}` + ); + } + this.messages.splice(index, 1); + } + + expectNoOtherMessages() { + equal(this.messages, [], "Expected no other messages to be logged."); + } + + override log(message: string, level: LogLevel): void { + this.messages.push(levelMap[level] + message); + } +} diff --git a/src/test/converter/exports/mod.ts b/src/test/converter/exports/mod.ts index 6d2786601..9781d2919 100644 --- a/src/test/converter/exports/mod.ts +++ b/src/test/converter/exports/mod.ts @@ -20,9 +20,7 @@ export { a as c } from "./mod"; /** * Will not be re-exported from export.ts using export * from... */ -export default function () { - console.log("Default"); -} +export default function () {} /** * This is annotated with the hidden tag and will therefore not be included in the generated documentation. diff --git a/src/test/converter2/issues/gh1547.ts b/src/test/converter2/issues/gh1547.ts index 972100b89..1000fa660 100644 --- a/src/test/converter2/issues/gh1547.ts +++ b/src/test/converter2/issues/gh1547.ts @@ -19,7 +19,5 @@ export class Test { * * @param things - Array of things or a thing. */ - log_thing(things: ValueOrArray): void { - console.log(things); - } + log_thing(things: ValueOrArray): void {} } diff --git a/src/test/converter2/issues/gh1580.ts b/src/test/converter2/issues/gh1580.ts index 63a048973..2caef2c83 100644 --- a/src/test/converter2/issues/gh1580.ts +++ b/src/test/converter2/issues/gh1580.ts @@ -5,9 +5,7 @@ class A { prop!: string; /** Run docs */ - run(): void { - console.log("A"); - } + run(): void {} } class B extends A { @@ -15,6 +13,5 @@ class B extends A { run(): void { super.run(); - console.log("B"); } } diff --git a/src/test/converter2/validation/class.ts b/src/test/converter2/validation/class.ts new file mode 100644 index 000000000..20689a559 --- /dev/null +++ b/src/test/converter2/validation/class.ts @@ -0,0 +1,8 @@ +export class Foo { + /** Comment */ + constructor(); + constructor(x: string); + constructor(x?: string) {} +} + +export class Bar {} diff --git a/src/test/converter2/validation/function.ts b/src/test/converter2/validation/function.ts new file mode 100644 index 000000000..f21370b09 --- /dev/null +++ b/src/test/converter2/validation/function.ts @@ -0,0 +1,7 @@ +/** Foo docs */ +export function foo() {} + +export function bar(); +/** Bar docs */ +export function bar(x: string); +export function bar() {} diff --git a/src/test/validation.test.ts b/src/test/validation.test.ts index 23e6bbb63..6c50dedfb 100644 --- a/src/test/validation.test.ts +++ b/src/test/validation.test.ts @@ -1,15 +1,12 @@ import { equal, fail, ok } from "assert"; import { join, relative } from "path"; import { Logger, LogLevel, normalizePath } from ".."; +import { validateDocumentation } from "../lib/validation/documentation"; import { validateExports } from "../lib/validation/exports"; import { getConverter2App, getConverter2Program } from "./programs"; +import { TestLogger } from "./TestLogger"; -function expectWarning( - typeName: string, - file: string, - referencingName: string, - intentionallyNotExported: readonly string[] = [] -) { +function convertValidationFile(file: string) { const app = getConverter2App(); const program = getConverter2Program(); const sourceFile = program.getSourceFile( @@ -26,6 +23,17 @@ function expectWarning( }, ]); + return project; +} + +function expectWarning( + typeName: string, + file: string, + referencingName: string, + intentionallyNotExported: readonly string[] = [] +) { + const project = convertValidationFile(file); + let sawWarning = false; const regex = /(.*?), defined at (.*?):\d+, is referenced by (.*?) but not included in the documentation\./; @@ -186,3 +194,38 @@ describe("validateExports", () => { ok(sawWarning, "Never saw warning."); }); }); + +describe("validateDocumentation", () => { + it("Should correctly handle functions", () => { + const project = convertValidationFile("function.ts"); + const logger = new TestLogger(); + validateDocumentation(project, logger, ["Function"]); + + logger.expectMessage( + "warn: bar, defined at src/test/converter2/validation/function.ts:4, does not have any documentation." + ); + logger.expectNoOtherMessages(); + }); + + it("Should correctly handle accessors", () => { + const project = convertValidationFile("getSignature.ts"); + const logger = new TestLogger(); + validateDocumentation(project, logger, ["Accessor"]); + + logger.expectMessage( + "warn: Foo.foo, defined at src/test/converter2/validation/getSignature.ts:2, does not have any documentation." + ); + logger.expectNoOtherMessages(); + }); + + it("Should correctly handle constructors", () => { + const project = convertValidationFile("class.ts"); + const logger = new TestLogger(); + validateDocumentation(project, logger, ["Constructor"]); + + logger.expectMessage( + "warn: Foo.constructor, defined at src/test/converter2/validation/class.ts:4, does not have any documentation." + ); + logger.expectNoOtherMessages(); + }); +});