Skip to content

Commit

Permalink
Keep type alias comments on the alias
Browse files Browse the repository at this point in the history
Resolves #2372
  • Loading branch information
Gerrit0 committed Dec 27, 2023
1 parent 7715f14 commit 03426c3
Show file tree
Hide file tree
Showing 9 changed files with 107 additions and 58 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
### Bug Fixes

- Methods which return function types no longer have duplicated comments, #2336.
- Comments on function-like type aliases will now show up under the type alias, rather than nested within the type declaration, #2372.
- Fix crash when converting some complicated union/intersection types, #2451.
- Navigation triangle markers should no longer display on a separate line with some font settings, #2457.
- `@group` and `@category` organization is now applied later to allow inherited comments to create groups/categories, #2459.
Expand Down
28 changes: 21 additions & 7 deletions src/lib/converter/plugins/CommentPlugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -387,13 +387,6 @@ export class CommentPlugin extends ConverterComponent {
? undefined
: reflection.comment;

// Since this reflection has signatures, remove the comment from the parent
// reflection. This is important so that in type aliases we don't end up with
// a comment rendered twice.
if (!reflection.kindOf(ReflectionKind.ClassOrInterface)) {
delete reflection.comment;
}

for (const signature of signatures) {
const childComment = (signature.comment ||= comment?.clone());
if (!childComment) continue;
Expand Down Expand Up @@ -450,6 +443,27 @@ export class CommentPlugin extends ConverterComponent {
childComment?.removeTags("@typeParam");
childComment?.removeTags("@template");
}

// Since this reflection has signatures, we need to remove the comment from the non-primary
// declaration location. For functions, this means removing it from the Function reflection.
// For type aliases, this means removing it from reflection.type.declaration.
// This is important so that in type aliases we don't end up with a comment rendered twice.
if (
reflection.kindOf(
ReflectionKind.FunctionOrMethod | ReflectionKind.Constructor,
)
) {
delete reflection.comment;
}
if (reflection.kindOf(ReflectionKind.TypeAlias)) {
reflection.comment?.removeTags("@param");
reflection.comment?.removeTags("@typeParam");
reflection.comment?.removeTags("@template");

for (const sig of signatures) {
delete sig.comment;
}
}
}

private removeExcludedTags(comment: Comment) {
Expand Down
30 changes: 24 additions & 6 deletions src/lib/validation/documentation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -56,19 +56,35 @@ export function validateDocumentation(
}
}

// Type aliases own their comments, even if they're function-likes.
// So if we're a type literal owned by a type alias, don't do anything.
if (
ref.kindOf(ReflectionKind.TypeLiteral) &&
ref.parent?.kindOf(ReflectionKind.TypeAlias)
) {
toProcess.push(ref.parent);
continue;
}
// Ditto for signatures on type aliases.
if (
ref.kindOf(ReflectionKind.CallSignature) &&
ref.parent?.parent?.kindOf(ReflectionKind.TypeAlias)
) {
toProcess.push(ref.parent.parent);
continue;
}

if (ref instanceof DeclarationReflection) {
const signatures =
ref.type instanceof ReflectionType
? ref.type.declaration.getNonIndexSignatures()
: ref.getNonIndexSignatures();

if (signatures.length) {
// We maybe used to have a comment, but the comment plugin has removed it.
// See CommentPlugin.onResolve. We've been asked to validate this reflection,
// (it's probably a type alias) so we should validate that signatures all have
// comments, but we shouldn't produce a warning here.
// We've been asked to validate this reflection, so we should validate that
// signatures all have comments, but we'll still have a comment here because
// type aliases always have their own comment.
toProcess.push(...signatures);
continue;
}
}

Expand All @@ -80,7 +96,9 @@ export function validateDocumentation(
}

logger.warn(
`${ref.getFriendlyFullName()}, defined in ${nicePath(
`${ref.getFriendlyFullName()} (${
ReflectionKind[ref.kind]
}), defined in ${nicePath(
symbolId.fileName,
)}, does not have any documentation.`,
);
Expand Down
16 changes: 8 additions & 8 deletions src/test/converter/alias/specs.json
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,14 @@
"variant": "declaration",
"kind": 2097152,
"flags": {},
"comment": {
"summary": [
{
"kind": "text",
"text": "A type that describes a compare function, e.g. for array.sort()."
}
]
},
"sources": [
{
"fileName": "alias.ts",
Expand Down Expand Up @@ -205,14 +213,6 @@
"variant": "signature",
"kind": 4096,
"flags": {},
"comment": {
"summary": [
{
"kind": "text",
"text": "A type that describes a compare function, e.g. for array.sort()."
}
]
},
"parameters": [
{
"id": 4,
Expand Down
16 changes: 8 additions & 8 deletions src/test/converter/js/specs.json
Original file line number Diff line number Diff line change
Expand Up @@ -685,6 +685,14 @@
"variant": "declaration",
"kind": 2097152,
"flags": {},
"comment": {
"summary": [
{
"kind": "text",
"text": "even though in the same comment block"
}
]
},
"sources": [
{
"fileName": "index.js",
Expand Down Expand Up @@ -716,14 +724,6 @@
"variant": "signature",
"kind": 4096,
"flags": {},
"comment": {
"summary": [
{
"kind": "text",
"text": "even though in the same comment block"
}
]
},
"type": {
"type": "intrinsic",
"name": "any"
Expand Down
32 changes: 16 additions & 16 deletions src/test/converter/mixin/specs.json
Original file line number Diff line number Diff line change
Expand Up @@ -1231,6 +1231,14 @@
"variant": "declaration",
"kind": 2097152,
"flags": {},
"comment": {
"summary": [
{
"kind": "text",
"text": "Any constructor function"
}
]
},
"sources": [
{
"fileName": "mixin.ts",
Expand Down Expand Up @@ -1275,14 +1283,6 @@
"variant": "signature",
"kind": 16384,
"flags": {},
"comment": {
"summary": [
{
"kind": "text",
"text": "Any constructor function"
}
]
},
"parameters": [
{
"id": 9,
Expand Down Expand Up @@ -1318,6 +1318,14 @@
"variant": "declaration",
"kind": 2097152,
"flags": {},
"comment": {
"summary": [
{
"kind": "text",
"text": "Any function"
}
]
},
"sources": [
{
"fileName": "mixin.ts",
Expand Down Expand Up @@ -1362,14 +1370,6 @@
"variant": "signature",
"kind": 4096,
"flags": {},
"comment": {
"summary": [
{
"kind": "text",
"text": "Any function"
}
]
},
"parameters": [
{
"id": 4,
Expand Down
5 changes: 5 additions & 0 deletions src/test/converter2/issues/gh2372.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
/**
* The signature for a function acting as an event handler.
* @param e Param should work
*/
export type EventHandler = (e: Event) => void;
29 changes: 20 additions & 9 deletions src/test/issues.c2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -414,16 +414,14 @@ describe("Issue Tests", () => {
it("#1734", () => {
const project = convert();
const alias = query(project, "Foo");
const type = alias.type;
ok(type instanceof ReflectionType);

const expectedComment = new Comment();
expectedComment.blockTags = [
new CommentTag("@asdf", [
{ kind: "text", text: "Some example text" },
]),
];
equal(type.declaration.signatures?.[0].comment, expectedComment);
equal(alias.comment, expectedComment);
});

it("#1745", () => {
Expand Down Expand Up @@ -565,12 +563,9 @@ describe("Issue Tests", () => {
equal(Type1.type?.type, "reflection" as const);
equal(Type2.type?.type, "reflection" as const);

equal(Type1.comment, new Comment([{ kind: "text", text: "On Tag" }]));
equal(
Type1.type.declaration.signatures?.[0].comment,
new Comment([{ kind: "text", text: "On Tag" }]),
);
equal(
Type2.type.declaration.signatures?.[0].comment,
Type2.comment,
new Comment([{ kind: "text", text: "Some type 2." }]),
);
});
Expand All @@ -579,7 +574,7 @@ describe("Issue Tests", () => {
const project = convert();
app.validate(project);
logger.expectMessage(
"warn: UnDocFn.__type, defined in */gh1898.ts, does not have any documentation.",
"warn: UnDocFn (TypeAlias), defined in */gh1898.ts, does not have any documentation.",
);
});

Expand Down Expand Up @@ -1166,6 +1161,22 @@ describe("Issue Tests", () => {
equal(ns?.children?.map((c) => c.name), ["property"]);
});

it("Puts delegate type alias comments on the type alias #2372", () => {
const project = convert();
equal(
getComment(project, "EventHandler"),
"The signature for a function acting as an event handler.",
);

const typeSig = query(project, "EventHandler").type?.visit({
reflection(r) {
return r.declaration.signatures![0];
},
});

equal(Comment.combineDisplayParts(typeSig?.comment?.summary), "");
});

it("Handles spaces in JSDoc default parameter names #2384", () => {
const project = convert();
const Typed = query(project, "Typed");
Expand Down
8 changes: 4 additions & 4 deletions src/test/validation.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -172,7 +172,7 @@ describe("validateDocumentation", () => {
validateDocumentation(project, logger, ["Function"]);

logger.expectMessage(
"warn: bar, defined in */function.ts, does not have any documentation.",
"warn: bar (CallSignature), defined in */function.ts, does not have any documentation.",
);
logger.expectNoOtherMessages();
});
Expand All @@ -183,7 +183,7 @@ describe("validateDocumentation", () => {
validateDocumentation(project, logger, ["Accessor"]);

logger.expectMessage(
"warn: Foo.foo, defined in */getSignature.ts, does not have any documentation.",
"warn: Foo.foo (GetSignature), defined in */getSignature.ts, does not have any documentation.",
);
logger.expectNoOtherMessages();
});
Expand All @@ -194,7 +194,7 @@ describe("validateDocumentation", () => {
validateDocumentation(project, logger, ["Constructor"]);

logger.expectMessage(
"warn: Foo.constructor, defined in */class.ts, does not have any documentation.",
"warn: Foo.constructor (ConstructorSignature), defined in */class.ts, does not have any documentation.",
);
logger.expectNoOtherMessages();
});
Expand All @@ -205,7 +205,7 @@ describe("validateDocumentation", () => {
validateDocumentation(project, logger, ["Method"]);

logger.expectMessage(
"warn: Foo.method, defined in */interface.ts, does not have any documentation.",
"warn: Foo.method (CallSignature), defined in */interface.ts, does not have any documentation.",
);
logger.expectNoOtherMessages();
});
Expand Down

0 comments on commit 03426c3

Please sign in to comment.