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

Signature help #1286

Closed
wants to merge 5 commits into from
Closed

Signature help #1286

wants to merge 5 commits into from

Conversation

rgrinberg
Copy link
Member

This series of patches were applied in LSP to provide signature hinting when a user types a function application. It would be much more convenient for us if they were upstreamed.

cc @mnxn

; active_param : int option
}

(* extract a properly properly parenthesized identifier
Copy link

Choose a reason for hiding this comment

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

Suggested change
(* extract a properly properly parenthesized identifier
(* extract a properly parenthesized identifier

Typo. oops.

Copy link
Collaborator

@voodoos voodoos left a comment

Choose a reason for hiding this comment

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

I started reading the changes, and I am wondering why you chose to return both the function signature as a string and the arguments offsets in that string. Wouldn't it be more simple to simply return the list of the arguments and then on the editor side you just print each argument with the correct style ?

@mnxn
Copy link

mnxn commented Apr 13, 2021

The function needs to build the string in order to get the parameter offsets.

There's some complexity in the printing (like unwrapping option types), that I wasn't sure was appropriate to do from the editor side.

@voodoos
Copy link
Collaborator

voodoos commented Feb 23, 2022

@mnxn before thinking about how we could improve and merge that PR I would like it would be good to have it's expected behaviour to be illustrated in a series of tests. Could you add some to the merlin test-suite ?

(you can take example from type-enclosing/type-alias.t for example)

The first cases that I can think about are:

  • a simple function with test for the first arg, second and last
  • a function with labelled argument that is then called with varying arguments order
  • a function with an optional argument
  • function calls before or after an error in the buffer

I noticed some weird behaviour while using vscode, I while try to take notes next time it happens to write more specific tests.

Thank you !

@mnxn
Copy link

mnxn commented Feb 23, 2022

My understanding is that to write a test like that example I would first need to expose the signature help functionality to the command line interface through the all_commands list.

Is that correct?

@voodoos
Copy link
Collaborator

voodoos commented Feb 24, 2022

Yes it is correct, and that's also required for a new Merlin feature: we eventually want it to be available for every Merlin's client and that means adding it to the protocol.

@voodoos
Copy link
Collaborator

voodoos commented Nov 7, 2022

Closing for now since this has been re-implemented downstream using Merlin as a library.

@voodoos voodoos closed this Nov 7, 2022
@3Rafal 3Rafal mentioned this pull request Jan 4, 2024
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