scztt / LanguageServer.quark

16 stars 10 forks source link

Add Hover Provider #27

Closed themissingcow closed 2 months ago

themissingcow commented 3 months ago

@scztt - I've been experimenting with adding docs support to the LSP via a hover provider. It's basically a rip-off of the HTML renderer adapted to output basic Markdown. Its no where near ready for merge yet. Is this was something you might be interested in merging once tidied?

Ignore all but the last commit (it's currently based on the work I've been doing with @dannyZyg to get things working in neovim. If it doesn't look like a totally bad idea (I'm relatively new to SC development), let me know and I can get it finished. Would always value any suggested improvements to better align with SC conventions too if you see anything that looks wrong.

Known issues:

Hover docs:

image

Suggestion docs

image

VSCode

image
scztt commented 2 months ago

Thanks much for this, I think this will be huge :) - I'll try to give some detailed code review notes in the coming days.

scztt commented 2 months ago

One initial comment: I think it would be very worthwhile to publish the Markdown renderer as a separate quark, because I think this would be extremely useful outside of the LSP project.

themissingcow commented 2 months ago

Thanks @scztt I'll try to get this over the line soon.

One initial comment: I think it would be very worthwhile to publish the Markdown renderer as a separate quark, because I think this would be extremely useful outside of the LSP project.

Great idea - I'll make it so.

themissingcow commented 2 months ago

One initial comment: I think it would be very worthwhile to publish the Markdown renderer as a separate quark, because I think this would be extremely useful outside of the LSP project.

I've split this out now:

https://github.com/themissingcow/SCDocMarkdownRenderer.quark

Somehow I had totally missed #13 though - which seems to use the same approach - @karnpapon wondering what the best way to combine this is? Looks like you also based things on the SCDocHTML renderer....

karnpapon commented 2 months ago

@themissingcow not quite sure how, also I'm relatively new for SC development (and LSP), but since we use the same approach it might not that hard to combine my version into yours, might try out this weekend.

themissingcow commented 2 months ago

@karnpapon Awesome, thanks. I could then focus on getting the Markdown renderer quark ready?

scztt commented 2 months ago

Glad you all are collaborating on this, it makes a lot of sense and I think the markdown renderer will be re-usable in other contexts.

Here's the approach I'd like to take - let me know how this sounds to you @themissingcow / @karnpapon? I'm very open to feedback and brainstorming here.

A new doc renderer is complex, and I imagine could take some time to get to a stable release point. However, the LSP's requirements for it are somewhat limited, and it would be useful even if it were in a "beta" state - but other users who need e.g. a bulletproof live performance setup might want to avoid things still under active development. With that in mind, I'd like to find a way to decouple the Markdown renderer from the language server, so that it work more like an optional plugin/extension - you can install/enable it if you want it, but you don't have to.

Another goal is that I want some of the LSP information providers like HoverProvider to be plugin-able anyway: e.g. I want you to be able to (potentially) hover over ~foo and get information about that object. With that in mind, I wonder if something like this would be the right approach:

  1. A generic api to provide documentation for a given object type, living in LanguageServer.quark

    DocumentationProvider {
    *registerProvider { |objectType, provider| } // provider is a Function/callable
    *hasProvider { |class| }
    *getDocumentation { |object, options| } 
    }
  2. The markdown renderer registers with this ONLY if DocumentationProvider is available (e.g. it's decoupled from the LanguageServer)

    \DocumentationProvider.asClass !? {
    |docProvider|
    docProvider.registerProvider(Class, { |class, options| SCDocMarkdownRenderer.render(class, options) });
    docProvider.registerProvider(Method, { |method, options| SCDocMarkdownRenderer.render(class, options) });
    }
  3. LSPDatabase forwards to DocumentationProvider, e.g. classDocString { |class| ^DocumentationProvider.getDocumentation(class) }, and (potentially) we disable HoverProvider if e.g. DocumentationProvider.hasProvider(Class) == false.

This means we can use the markdown renderer without LSP, and LSP without the markdown provider, AND we can provide additional documentation providers in the future without changing the LSP. There are some pieces missing for the more advanced version of this, but I think decoupling LSP and the markdown renderer is a good start.

Thoughts?

themissingcow commented 2 months ago

This means we can use the markdown renderer without LSP, and LSP without the markdown provider, AND we can provide additional documentation providers in the future without changing the LSP. There are some pieces missing for the more advanced version of this, but I think decoupling LSP and the markdown renderer is a good start.

Thoughts?

Sounds spot on to me @scztt 👌

themissingcow commented 2 months ago

Closing, as development of the MD renderer will go on else where, and the actual integration into the LSP needs to take a different shape...

karnpapon commented 2 months ago

agreed, decoupling the renderer from LSP might be the way to go.

themissingcow commented 2 months ago

So shall I focus on tidying up the renderer a little bit (I know of a few things left to do)? @karnpapon do you have any time to look at @scztt's DocumentationProvider abstraction?

karnpapon commented 2 months ago

@themissingcow I do have some time for this. However, since my last involvement was around almost a year ago, it may take some time for me to reacquaint myself with the details. I will keep you updated on my progress.