Skip to content

Commit

Permalink
intentionallyNotExported now respects qualified names
Browse files Browse the repository at this point in the history
 Resolves #1972.
  • Loading branch information
Gerrit0 committed Jun 30, 2022
1 parent 4367d51 commit eb8bf05
Show file tree
Hide file tree
Showing 12 changed files with 89 additions and 88 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@

- TypeDoc will no longer crash if a comment contains an empty `@example` tag, #1967.
- TypeDoc will now detect attempted inheritance from accessors and inherit from the getter or setter, #1968.
- `intentionallyNotExported` will now properly respect qualified names, #1972.
- Validation warnings caused by missing documentation will now be formatted like other warnings which reference a declaration.

### Features

Expand Down
26 changes: 22 additions & 4 deletions src/lib/converter/comments/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -198,10 +198,28 @@ function blockTag(

const tagName = aliasedTags.get(blockTag.text) || blockTag.text;

return new CommentTag(
tagName as `@${string}`,
blockContent(comment, lexer, config, warning)
);
let content: CommentDisplayPart[];
if (tagName === "@example") {
content = exampleBlockContent(comment, lexer, config, warning);
} else {
content = blockContent(comment, lexer, config, warning);
}

return new CommentTag(tagName as `@${string}`, content);
}

/**
* The `@example` tag gets a special case because otherwise we will produce many warnings
* about unescaped/mismatched/missing braces
*/
function exampleBlockContent(
comment: Comment,
lexer: LookaheadGenerator<Token>,
config: CommentParserConfig,
warning: (msg: string) => void
): CommentDisplayPart[] {
// TODO: Needs implementation
return blockContent(comment, lexer, config, warning);
}

function blockContent(
Expand Down
30 changes: 10 additions & 20 deletions src/lib/models/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { Reflection } from "./reflections/abstract";
import type { DeclarationReflection } from "./reflections/declaration";
import type { ProjectReflection } from "./reflections/project";
import type { Serializer, JSONOutput } from "../serialization";
import { getQualifiedName } from "../utils/tsutils";

/**
* Base class of all type definitions.
Expand Down Expand Up @@ -796,9 +797,8 @@ export class ReferenceType extends Type {
/**
* The fully qualified name of the referenced type, relative to the file it is defined in.
* This will usually be the same as `name`, unless namespaces are used.
* Will only be set for `ReferenceType`s pointing to a symbol within `node_modules`.
*/
qualifiedName?: string;
qualifiedName: string;

/**
* The package that this type is referencing.
Expand All @@ -812,20 +812,22 @@ export class ReferenceType extends Type {
private constructor(
name: string,
target: ts.Symbol | Reflection | number,
project: ProjectReflection | null
project: ProjectReflection | null,
qualifiedName: string
) {
super();
this.name = name;
this._target = target instanceof Reflection ? target.id : target;
this._project = project;
this.qualifiedName = qualifiedName;
}

static createResolvedReference(
name: string,
target: Reflection | number,
project: ProjectReflection | null
) {
return new ReferenceType(name, target, project);
return new ReferenceType(name, target, project, name);
}

static createSymbolReference(
Expand All @@ -836,7 +838,8 @@ export class ReferenceType extends Type {
const ref = new ReferenceType(
name ?? symbol.name,
symbol,
context.project
context.project,
getQualifiedName(context.checker, symbol)
);

const symbolPath = symbol?.declarations?.[0]
Expand All @@ -856,25 +859,12 @@ export class ReferenceType extends Type {
const packageName = symbolPath.substring(startIndex, stopIndex);
ref.package = packageName;

const qualifiedName = context.checker.getFullyQualifiedName(symbol);
// I think this is less bad than depending on symbol.parent...
// https://github.com/microsoft/TypeScript/issues/38344
// It will break if someone names a directory with a quote in it, but so will lots
// of other things including other parts of TypeDoc. Until it *actually* breaks someone...
if (qualifiedName.startsWith('"')) {
ref.qualifiedName = qualifiedName.substring(
qualifiedName.indexOf('".', 1) + 2
);
} else {
ref.qualifiedName = qualifiedName;
}

return ref;
}

/** @internal this is used for type parameters, which don't actually point to something */
static createBrokenReference(name: string, project: ProjectReflection) {
return new ReferenceType(name, -1, project);
return new ReferenceType(name, -1, project, name);
}

protected override getTypeString() {
Expand Down Expand Up @@ -904,7 +894,7 @@ export class ReferenceType extends Type {
name: this.name,
};

if (this.qualifiedName && this.package) {
if (this.package) {
result.qualifiedName = this.qualifiedName;
result.package = this.package;
}
Expand Down
6 changes: 2 additions & 4 deletions src/lib/serialization/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,11 +243,9 @@ export interface QueryType extends Type, S<M.QueryType, "type" | "queryType"> {}

export interface ReferenceType
extends Type,
S<
M.ReferenceType,
"type" | "name" | "typeArguments" | "qualifiedName" | "package"
> {
S<M.ReferenceType, "type" | "name" | "typeArguments" | "package"> {
id?: number;
qualifiedName?: string;
}

export interface ReflectionType extends Type, S<M.ReflectionType, "type"> {
Expand Down
2 changes: 2 additions & 0 deletions src/lib/utils/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,3 +64,5 @@ export { JSX };
export { Fragment, Raw, renderElement } from "./jsx";

export * as Validation from "./validation";

export * from "./tsutils";
14 changes: 14 additions & 0 deletions src/lib/utils/tsutils.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
import type * as ts from "typescript";

export function getQualifiedName(checker: ts.TypeChecker, symbol: ts.Symbol) {
const qualifiedName = checker.getFullyQualifiedName(symbol);
// I think this is less bad than depending on symbol.parent...
// https://github.com/microsoft/TypeScript/issues/38344
// It will break if someone names a directory with a quote in it, but so will lots
// of other things including other parts of TypeDoc. Until it *actually* breaks someone...
if (qualifiedName.startsWith('"') && qualifiedName.includes('".')) {
return qualifiedName.substring(qualifiedName.indexOf('".', 1) + 2);
} else {
return qualifiedName;
}
}
16 changes: 3 additions & 13 deletions src/lib/validation/documentation.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import * as path from "path";
import * as ts from "typescript";
import {
DeclarationReflection,
ProjectReflection,
Expand All @@ -8,7 +6,7 @@ import {
ReflectionType,
SignatureReflection,
} from "../models";
import { Logger, normalizePath } from "../utils";
import type { Logger } from "../utils";
import { removeFlag } from "../utils/enum";

export function validateDocumentation(
Expand Down Expand Up @@ -86,17 +84,9 @@ export function validateDocumentation(
continue;
}

const { line } = ts.getLineAndCharacterOfPosition(
sourceFile,
decl.getStart()
);
const file = normalizePath(
path.relative(process.cwd(), sourceFile.fileName)
);

const loc = `${file}:${line + 1}`;
logger.warn(
`${ref.getFriendlyFullName()}, defined at ${loc}, does not have any documentation.`
`${ref.getFriendlyFullName()} does not have any documentation.`,
decl
);
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/lib/validation/exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ function makeIntentionallyExportedHelper(
});

return {
has(symbol: ts.Symbol) {
has(symbol: ts.Symbol, typeName: string) {
// If it isn't declared anywhere, we can't produce a good error message about where
// the non-exported symbol is, so even if it isn't ignored, pretend it is. In practice,
// this will happen incredibly rarely, since symbols without declarations are very rare.
Expand All @@ -52,7 +52,7 @@ function makeIntentionallyExportedHelper(

for (const [index, [file, name]] of processed.entries()) {
if (
symbol.name === name &&
typeName === name &&
symbol.declarations.some((d) =>
d.getSourceFile().fileName.endsWith(file)
)
Expand Down Expand Up @@ -90,15 +90,15 @@ export function validateExports(
if (!type.reflection && symbol) {
if (
(symbol.flags & ts.SymbolFlags.TypeParameter) === 0 &&
!intentional.has(symbol) &&
!intentional.has(symbol, type.qualifiedName) &&
!warned.has(symbol)
) {
warned.add(symbol);
const decl = symbol.declarations![0];

logger.warn(
`${
type.name
type.qualifiedName
} is referenced by ${current!.getFullName()} but not included in the documentation.`,
decl
);
Expand Down
9 changes: 7 additions & 2 deletions src/test/TestLogger.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { Logger, LogLevel, removeIf } from "../lib/utils";
import { deepStrictEqual as equal, fail } from "assert";
import { fail, ok } from "assert";
import * as ts from "typescript";
import { resolve } from "path";

Expand Down Expand Up @@ -36,7 +36,12 @@ export class TestLogger extends Logger {
}

expectNoOtherMessages() {
equal(this.messages, [], "Expected no other messages to be logged.");
ok(
this.messages.length === 0,
`Expected no other messages to be logged. The logged messages were:\n\t${this.messages.join(
"\n\t"
)}`
);
}

override diagnostic(diagnostic: ts.Diagnostic): void {
Expand Down
5 changes: 5 additions & 0 deletions src/test/converter2/validation/namespace.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
export type Foo = Bar.Baz;

namespace Bar {
export type Baz = 123;
}
2 changes: 1 addition & 1 deletion src/test/issueTests.ts
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,7 @@ export const issueTests: {
app.validate(project);
logger.discardDebugMessages();
logger.expectMessage(
"warn: UnDocFn.__type, defined at src/test/converter2/issues/gh1898.ts:4, does not have any documentation."
"warn: UnDocFn.__type does not have any documentation."
);
logger.expectNoOtherMessages();
},
Expand Down
57 changes: 17 additions & 40 deletions src/test/validation.test.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { equal, fail, ok } from "assert";
import { ok } from "assert";
import { join } from "path";
import { Logger, LogLevel } from "..";
import { validateDocumentation } from "../lib/validation/documentation";
Expand Down Expand Up @@ -34,27 +34,12 @@ function expectWarning(
) {
const project = convertValidationFile(file);

let sawWarning = false;
const regex =
/(.*?) is referenced by (.*?) but not included in the documentation\./;

class LoggerCheck extends Logger {
override log(message: string, level: LogLevel) {
const match = message.match(regex);
if (level === LogLevel.Warn && match) {
sawWarning = true;
equal(match[1], typeName, "Missing type name is different.");
equal(
match[2],
referencingName,
"Referencing name is different"
);
}
}
}
const logger = new TestLogger();
validateExports(project, logger, intentionallyNotExported);

validateExports(project, new LoggerCheck(), intentionallyNotExported);
ok(sawWarning, `Expected warning message for ${typeName} to be reported.`);
logger.expectMessage(
`warn: ${typeName} is referenced by ${referencingName} but not included in the documentation.`
);
}

function expectNoWarning(
Expand All @@ -77,19 +62,11 @@ function expectNoWarning(
},
]);

const regex =
/(.*?), defined at (.*?):\d+, is referenced by (.*?) but not included in the documentation\./;

class LoggerCheck extends Logger {
override log(message: string, level: LogLevel) {
const match = message.match(regex);
if (level === LogLevel.Warn && match) {
fail("Expected no warnings about missing exports");
}
}
}
const logger = new TestLogger();

validateExports(project, new LoggerCheck(), intentionallyNotExported);
validateExports(project, logger, intentionallyNotExported);
logger.discardDebugMessages();
logger.expectNoOtherMessages();
}

describe("validateExports", () => {
Expand Down Expand Up @@ -143,6 +120,10 @@ describe("validateExports", () => {
expectNoWarning("externalType.ts");
});

it("Should not warn if namespaced name is given to intentionallyNotExported", () => {
expectNoWarning("namespace.ts", ["Bar.Baz"]);
});

it("Should warn if intentionallyNotExported contains unused values", () => {
const app = getConverter2App();
const program = getConverter2Program();
Expand Down Expand Up @@ -191,9 +172,7 @@ describe("validateDocumentation", () => {
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.expectMessage("warn: bar does not have any documentation.");
logger.expectNoOtherMessages();
});

Expand All @@ -202,9 +181,7 @@ describe("validateDocumentation", () => {
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.expectMessage("warn: Foo.foo does not have any documentation.");
logger.expectNoOtherMessages();
});

Expand All @@ -214,7 +191,7 @@ describe("validateDocumentation", () => {
validateDocumentation(project, logger, ["Constructor"]);

logger.expectMessage(
"warn: Foo.constructor, defined at src/test/converter2/validation/class.ts:4, does not have any documentation."
"warn: Foo.constructor does not have any documentation."
);
logger.expectNoOtherMessages();
});
Expand Down

0 comments on commit eb8bf05

Please sign in to comment.