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

Add textDocument/inlineValue from LSP 3.17 #1042

Merged
merged 2 commits into from
Feb 15, 2023
Merged

Add textDocument/inlineValue from LSP 3.17 #1042

merged 2 commits into from
Feb 15, 2023

Conversation

kaashyapan
Copy link
Contributor

Serve information from fSharp/pipelineHints via the new LSP endpoint textDocument/inlineValue (LSP spec 3.17)

@kaashyapan
Copy link
Contributor Author

@kaashyapan
Copy link
Contributor Author

Capture5

@baronfel
Copy link
Contributor

sorry for the delay on this - this is awesome! I've kicked off the pipelines on both repos and would love to help bring this in

@baronfel
Copy link
Contributor

When we switch to this, are there changes to the Ionide configs that are required? Or should this change also listen to the Ionide settings around pipeline hints and turn itself on/off with those?

@kaashyapan
Copy link
Contributor Author

I dont think any change to the config is required.
My understanding is that pipeline hints is a config that is only read on the client side and can be handled by existing options.

https://github.com/tboby/ionide-vscode-fsharp/blob/3272c18673c8813cf6ce7890764e89a04d6cd67b/src/Components/PipelineHints.fs#L16

@baronfel
Copy link
Contributor

If this new LSP capability is added to FSAC, won't the existing pipelinehints functionality in Ionide override/conflict with this then? That's what I'm getting at. If both mechanisms deliver the same content, then only one of them should run. And if we allowed configuration of the mechanism via config in Ionide before, then the new mechanism should respect that configuration as well.

@kaashyapan
Copy link
Contributor Author

You are right.

  1. The newly acquired inlineValue server capability and client capability should decide whether the new endpoint gets used. There would be no conflict because
  • I have not made the change to expose the new inlineValue server capability yet.
  • There would be no conflict in ionide-vscode because The LSP client js library is 6.* and the inline value client capability is added in 8.* (I think).
    .
  1. I think it is best to serve the same information on 2 endpoints until all the clients (vim & emacs) can switch over to the LSP endpoint and we can deprecate fsharp/pipelinehints on the server in the future (after ionide-vscode also catches up).

  2. The existing config option is purely on the client side and no change is required on account of the new LSP endpoint. The config option should be respected on the client side and the client orchestrate accordingly (i.e use any one endpoint). If the LSP client has the enabled the inline value capability it should not use the old end point.

If you agree on the above 3 points I will make the change to broadcast this new server capability.

@baronfel
Copy link
Contributor

I think we're on version 8 of the languageclient: https://github.com/ionide/ionide-vscode-fsharp/blob/main/package.json#L20

But that's not a huge deal - if I'm understanding you correctly, if you broadcast this capability, and check if the client supports the capability, then this endpoint will be toggled off/on, and we can control which mechanism (Ionide client-side pipeline hints or the new inlineValues) are used at the Ionide level instead when we update the FSAC used by Ionide. Is that correct? If so, then I agree with your plan entirely.

@Krzysztof-Cieslak
Copy link
Member

This is really cool, could you rebase on the latest changes?

@baronfel
Copy link
Contributor

Looks great, and the test failures are that one silly flaky test. Thanks for this @kaashyapan!

@baronfel baronfel merged commit fdeca2f into ionide:main Feb 15, 2023
@baronfel
Copy link
Contributor

@kaashyapan what did you have to do to get the inline values to show? I noticed that the inlineValueToggle wasn't hooked into the server capabilities on either Adaptive or 'normal' LSP while I was trying to plumb the new inlineValues.[enabled, prefix] settings into Ionide, but even after hooking those up I couldn't get any textDocument/inlineValue calls to be triggered by the client.

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