Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Fix for rest parameters in unified-signatures #2342

Merged
merged 4 commits into from
Mar 14, 2017
Merged

Fix for rest parameters in unified-signatures #2342

merged 4 commits into from
Mar 14, 2017

Conversation

sbj42
Copy link
Contributor

@sbj42 sbj42 commented Mar 14, 2017

PR checklist

Overview of change:

signaturesDifferByOptionalOrRestParameter() previously considered two signatures similar if the common parameters had the same type, but it did not consider the dotDotDotToken sigil when it dd that comparison. This change has the function check the last parameter of the shorter signature to see if it is a rest parameter. If so, it may not be possible to combine the two signatures.

Is there anything you'd like reviewers to focus on?

This change makes tslint more lenient about a particular scenario:

function rest2(...xs: number[]): void;
function rest2(xs: number[], y: number): void;
function rest2(...args: any[]) {}

tslint previously correctly noted that those signatures could be combined. After this change it does not warn about them. Reviewer should consider whether fixing the false warning of #2338 in exchange for failing to warn in this alternative case is a net improvement.

[bugfix] unified-signatures now recognizes rest parameters

`unified-signatues` erroneously claims overloads can be combined
Have `signaturesDifferByOptionalOrRestParameter()` use `parametersAreEqual()` to test the non-extra parameters.
This focuses the fix on `dotDotDotToken`, which if present can only be the final token of the shorter signature.  The previous fix also considered `questionToken`, which made a different case pass when it should have failed.
@nchen63
Copy link
Contributor

nchen63 commented Mar 14, 2017

looks good. thanks

@nchen63 nchen63 merged commit 3741729 into palantir:master Mar 14, 2017
@sbj42 sbj42 deleted the issue2338 branch March 14, 2017 17:49
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.

2 participants