Skip to content

Add signature help to Typescript server#2480

Merged
DanielRosenwasser merged 3 commits into
microsoft:masterfrom
dbaeumer:feature/signatureHelp
Mar 25, 2015
Merged

Add signature help to Typescript server#2480
DanielRosenwasser merged 3 commits into
microsoft:masterfrom
dbaeumer:feature/signatureHelp

Conversation

@dbaeumer
Copy link
Copy Markdown
Member

Enclosed a pull request to add signature help support to the TypeScript server. I tried to follow the style I found in the existing code (e.g. expose the language service API, no extra abstraction).

There is one thing I left open: I don't know the meaning of applicableSpan in SignatureHelpItems. Can someone of you fill in the comment.

Thanks Dirk

@msftclas
Copy link
Copy Markdown

Hi @dbaeumer, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!


It looks like you're a Microsoft contributor (Dirk Baeumer). If you're full-time, we DON'T require a Contribution License Agreement. If you are a vendor, please DO sign the electronic Contribution License Agreement. It will take 2 minutes and there's no faxing! https://cla.microsoft.com.

TTYL, MSBOT;

Comment thread src/server/session.ts Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Add a space after the colon

@DanielRosenwasser
Copy link
Copy Markdown
Member

Hey @dbaeumer, applicableSpan is simply the span for which signature help should appear on a signature - I believe the idea is that if you request signature help and move your cursor to another location within that span, you should continue seeing the same signature help dialog. I don't know whether this applies to mouse click into a span; I believe it is just for keyboard movement, though I can check and let you know if this is not the case.

For details on applicable spans, see the applicable span calculation for call expressions and for tagged templates.

Comment thread src/server/session.ts Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

You guys seem to do something similar throughout the function, you might want to consider just making a helper that returns a {start: number, end: number}.

@DanielRosenwasser
Copy link
Copy Markdown
Member

Apart from the nit about the space after the colon, this looks good to me. I'll let @steveluc give the 👍 though.

@steveluc
Copy link
Copy Markdown
Contributor

This is great. Thanks. Could you consider changing the name to SignatureHelpParameter to SignatureHelpArgs. The rest are named with the Args suffix.

@steveluc
Copy link
Copy Markdown
Contributor

Disregard previous comment. I see the naming is consistent and SignatureHelpParameter is part of the response.

👍

Comment thread src/server/session.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Put the comment on the wrong PR. so putting it here again :)

Can you also update client.ts and add a test for signature help in tests\cases\fourslash\server. I would copy a signature help test from tests\cases\fourslash to start.

@dbaeumer
Copy link
Copy Markdown
Member Author

Thanks for all the comments. Will work on the test and fix the typos and provide an updated. pull request.

@mhegazy
Copy link
Copy Markdown
Contributor

mhegazy commented Mar 25, 2015

👍

DanielRosenwasser added a commit that referenced this pull request Mar 25, 2015
Add signature help to Typescript server
@DanielRosenwasser DanielRosenwasser merged commit e33b24d into microsoft:master Mar 25, 2015
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
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.

5 participants