Skip to content
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

feat(constructSignature): add construct signature #116

Merged
merged 4 commits into from
Dec 3, 2019

Conversation

Pmyl
Copy link
Collaborator

@Pmyl Pmyl commented Dec 3, 2019

I'm going to treat construct signature in the same way as call signature.

Something doesn't work perfectly but I think it's a non-problem:

interface Test {
  new (): string;
}

is an accepted signature but my implementation can handle only constructions of object-like values.

Let me know if this concerns you

@Pmyl Pmyl requested a review from uittorio December 3, 2019 19:33
case ts.SyntaxKind.CallSignature:
return GetFunctionTypeDescriptor(node as ts.CallSignatureDeclaration, scope);
return GetFunctionTypeDescriptor(node as ts.CallSignatureDeclaration | ts.ConstructSignatureDeclaration, scope);
Copy link
Member

Choose a reason for hiding this comment

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

I think you can remove the OR

Copy link
Member

Choose a reason for hiding this comment

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

Ah sorry. I didn't see that you may still want to keep the switch case separated. Once you've done that you can remove the OR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put the OR there to make sure the typings are correct... if for some reason we change GetFunctionTypeDescriptor to only accept CallSignatureDeclaration it would give error... if I remove one of the two then there is no check anymore.

Do you want me to split the 2 cases of the switch?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I split the 2 cases

@Pmyl Pmyl merged commit b0aa18a into master Dec 3, 2019
@uittorio uittorio deleted the add_construct_signature branch December 27, 2019 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants