Skip to content

Show expanded parameter lists in signature help #30299

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 1 commit into from
Mar 9, 2019

Conversation

ahejlsberg
Copy link
Member

@ahejlsberg ahejlsberg commented Mar 9, 2019

The PR fixes signature help to show expanded parameter lists for rest parameters of tuple types (reported here #30215 (comment)). For example, instead of (...args: [string]) => string we now show (a: string) => string. This is similar to what we already do in quick info.

Copy link
Member

@weswigham weswigham left a comment

Choose a reason for hiding this comment

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

We should probably add a fourslash test this affects.

@ahejlsberg
Copy link
Member Author

@weswigham Yes, except I've spent enough time in fourslash hell to last a lifetime.

@ahejlsberg ahejlsberg merged commit 8e2a154 into master Mar 9, 2019
@ahejlsberg ahejlsberg deleted the showExpandedParameters branch March 9, 2019 21:16
@dragomirtitian
Copy link
Contributor

Guess this #30084 is no longer needed then :( @ahejlsberg

@@ -574,7 +574,7 @@ namespace ts.SignatureHelp {
printer.writeList(ListFormat.TypeParameters, args, sourceFile, writer);
}
});
const parameters = candidateSignature.parameters.map(p => createSignatureHelpParameterForParameter(p, checker, enclosingDeclaration, sourceFile, printer));
const parameters = checker.getExpandedParameters(candidateSignature).map(p => createSignatureHelpParameterForParameter(p, checker, enclosingDeclaration, sourceFile, printer));
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if this matters or not, but isVariadic will be true for functions with expanded parameters even though the expanded signature will not always be. Not sure if this will be an issue, in my similar PR, I reevaluated whether the signature is variadic based on the last argument. I found this with the fourslash test I added, don't know if any clients of the API will suffer from isVariadic not being entirely acurate in this case.

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't use the isVariadic property ourselves but it is part of the public API so we should make sure it is set correctly.

@ahejlsberg
Copy link
Member Author

@dragomirtitian Sorry, I completely missed that you already had created a PR for this issue. Yours has tests and handles the isVariadic flag so I still think we should get it merged. I will comment more in the issue itself.

@dragomirtitian
Copy link
Contributor

@ahejlsberg No problem these things happen :). I'm happy if you think I can still contribute in some way. I will fix the PR shortly.

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.

3 participants