Skip to content

[GH-43213] Fix duplicate comments printed in quick info section #43240

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

Merged
merged 7 commits into from
Mar 19, 2021

Conversation

hahmadia
Copy link
Contributor

@hahmadia hahmadia commented Mar 14, 2021

Fixes #43213

Only push values to the comments array if it's unique (similar to how we do it for declartation comments in let doc = JsDoc.getJsDocCommentsFromDeclarations)

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Mar 14, 2021
@typescript-bot
Copy link
Collaborator

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

@typescript-bot typescript-bot added For Backlog Bug PRs that fix a backlog bug and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Mar 14, 2021
@orta orta requested a review from sandersn March 14, 2021 22:50
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An idea for a simpler approach.

@@ -584,7 +584,17 @@ namespace ts {
forEachUnique(declarations, declaration => {
const inheritedDocs = findBaseOfDeclaration(checker, declaration, symbol => symbol.getDocumentationComment(checker));
// TODO: GH#16312 Return a ReadonlyArray, avoid copying inheritedDocs
if (inheritedDocs) doc = doc.length === 0 ? inheritedDocs.slice() : inheritedDocs.concat(lineBreakPart(), doc);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need to write your own loop; findBaseOfDeclaration allows you to skip base declarations by returning undefined, so you can maintain seen: Set<Symbol> to allow you to skip a symbol that's been seen before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, the outer forEachUnique might be redundant with this fix in place. Might be worth running tests once with it removed to see.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

GENUIS!

@hahmadia hahmadia requested review from sandersn March 19, 2021 18:10
@hahmadia
Copy link
Contributor Author

@sandersn
I've addressed your comments.

@hahmadia
Copy link
Contributor Author

I removed the forEachUnique since it wasn't really doing much in this case and replaced it with a normal for each loop.

@sandersn sandersn merged commit 6a83ae1 into microsoft:master Mar 19, 2021
@hahmadia hahmadia deleted the GH-43213 branch March 19, 2021 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Duplicate JSDoc in union of derived interfaces
3 participants