Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Apr 19, 2018

Fixes #23529

@ghost ghost requested review from sandersn and sheetalkamat April 19, 2018 19:40
@ghost ghost force-pushed the signatureHelpArrowFunction branch from f8ccc4a to 1718330 Compare April 19, 2018 22:35
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.

Seems fine, but I have one question about the early-exit refactoring. (plus one style nit)

(parent.kind === SyntaxKind.PropertyAssignment ||
parent.kind === SyntaxKind.PropertyDeclaration ||
getNestedModuleDeclaration(parent))) {
if (!parent) return;
Copy link
Member

Choose a reason for hiding this comment

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

why doesn't this miss a lot of tags, for example, when node.kind === SyntaxKind.Parameter on line 1848 and following?

(getSingleInitializerOfVariableStatementOrPropertyDeclaration(parent.parent.parent) === node || getSourceOfDefaultedAssignment(parent.parent.parent))) {
if (parent.parent && parent.parent.parent &&
(getSingleVariableOfVariableStatement(parent.parent.parent)
|| getSingleInitializerOfVariableStatementOrPropertyDeclaration(parent.parent.parent) === node
Copy link
Member

Choose a reason for hiding this comment

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

should be trailing, not leading, || because of our ASI-avoidance rules.

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.

Duh, you reminded me that the only thing with no parent is a source file, which doesn't have tags.

@ghost ghost merged commit 94cc59c into master Apr 20, 2018
@ghost ghost deleted the signatureHelpArrowFunction branch April 20, 2018 16:43
@microsoft microsoft locked and limited conversation to collaborators Jul 30, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant