Skip to content

Signature help #547

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 24 commits into from
Sep 22, 2022
Merged

Signature help #547

merged 24 commits into from
Sep 22, 2022

Conversation

zth
Copy link
Collaborator

@zth zth commented Aug 6, 2022

Closes #252

This implements signature help for function applications. Signature help is an "interactive" help hint you get when calling a function, that tells you what parameters the function has, as well as what parameter you're currently filling in, if any. Essentially a hover, but with the current parameter you're working on highlighted.

image

https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#textDocument_signatureHelp

@zth zth requested a review from cristianoc August 6, 2022 18:26
@zth
Copy link
Collaborator Author

zth commented Aug 6, 2022

It's ready for a general review, although I have a couple of more cases to handle here in larger functions.

@zth
Copy link
Collaborator Author

zth commented Aug 7, 2022

After some testing, this has some ways to go for larger function bodies. I had another idea - instead of printing a dummy function, extracting types, and calculating offsets that way, what if I simply take the label (type string) that will be the signature help, parse that type definition with the parser again, and let the parser tell me all of the offsets.

I think that should work, and also future proof/vastly simplify things.

@cristianoc
Copy link
Collaborator

Sounds like a good thing to try.

Comment on lines +210 to +211
Printf.sprintf ",\n \"documentation\": %s"
(stringifyMarkupContent docs))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Poor mans optional prop stringification.

@zth
Copy link
Collaborator Author

zth commented Aug 7, 2022

@cristianoc ready for your eyes.

…ight that as active, no matter what is picked up at the cursor
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

This looks great.
Only some high level comments -- the general structure and tests look good.

Still have to play with this, only looked at the code.

Question: should this be active by default or under a config? Any thought of which one it should be?

@cristianoc
Copy link
Collaborator

cristianoc commented Aug 8, 2022

Compared to other features, this is a little more elaborate i.e. maintenance cost for the future.
I'd say the ratio value / cost is OKish.

@zth
Copy link
Collaborator Author

zth commented Sep 15, 2022

Will pick up this again and finish it soon.

@zth
Copy link
Collaborator Author

zth commented Sep 19, 2022

@cristianoc ready for your eyes again.

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Got some philosophical question.

Also how about when named and unnamed arguments are mixed in the same call: does that work at the moment?

@zth
Copy link
Collaborator Author

zth commented Sep 22, 2022

Got some philosophical question.

Also how about when named and unnamed arguments are mixed in the same call: does that work at the moment?

It should, there are a few tests for it. Although I haven't tested it extensively.

@zth
Copy link
Collaborator Author

zth commented Sep 22, 2022

@cristianoc would you mind having another look?

@zth zth requested a review from cristianoc September 22, 2022 10:39
Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Would you update the change log before merging.

@zth zth merged commit 36dbc3e into master Sep 22, 2022
@zth zth deleted the signature-help branch September 22, 2022 11:13
zth added a commit that referenced this pull request Sep 28, 2022
* wire up server capability + command for signature help

* set up analysis bin command, and needed things from the LS protocol

* set up empty signature help test

* another test to help distinguish

* implement signature help for function applications

* changelog + readme

* leverage the parser to figure out the parameter offsets

* include docs for signature if available

* if a function has only one (unlabelled) argument, we can always highlight that as active, no matter what is picked up at the cursor

* add debug utilities

* get rid of hasPosInclusive

* move extractExpApplyArgs to SharedTypes for real

* shared function

* add way to parse via raw source string in parser, and replace current temp file mechanism for parsing signature help type label

* refactor logic some

* refactor hover so type expansion can be shared, and integrate into signature help

* gate new experimental signature help behind configuration option

* comment

* changelog
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.

Signature help on function applications
2 participants