scalameta / metals-sublime

Sublime Text package for Metals, a language server for Scala
https://packagecontrol.io/packages/LSP-metals
Apache License 2.0
16 stars 10 forks source link

Send range with textDocument/hover #45

Closed ayoub-benali closed 2 years ago

ayoub-benali commented 2 years ago

This is needed to handle https://github.com/scalameta/metals/pull/3060 It should be handle by LSP package once https://github.com/microsoft/language-server-protocol/issues/377 is resolved

Peek 2021-11-14 21-54

@rwols @rchl I duplicated some code that is in hover.py. If you think this behavior change should in https://github.com/sublimelsp/LSP let me know.

Todo:

rchl commented 2 years ago

I don't mind this being done here but I guess it doesn't handle the range case when hovering the file with mouse.

I think I wouldn't mind allowing the AbstractPlugin an ability to define special experimental flags that would change the behavior of LSP.

For example something like an additional API method:

def get_experimental_flags(self): dict:
  return {}

That the plugin could override and return something like { 'hover_supports_range': True } which would enable special logic for that specific session.

But before adding something like that, @rwols should also state his opinion.

ayoub-benali commented 2 years ago

The downside of this approach is that it creates a new command that very similar to the base lsp_hover which might be confusing. I created https://github.com/sublimelsp/LSP/pull/1898 which should provide a better experience.

ayoub-benali commented 2 years ago

Will try to handle this in https://github.com/sublimelsp/LSP rather adding a custom new command

ckipp01 commented 2 years ago

Will try to handle this in https://github.com/sublimelsp/LSP rather adding a custom new command

Again, sorry for confusing this convo and the other one 😆 I need to pay more attention to what repo I'm on. But I'd actually recommend just keeping the implementation in here and not adding it to the core. It's very metals specific, so I totally get the hesitation on the Sublime side to add that, especially because of the way we implemented it.

rchl commented 2 years ago

Implementation would be very easy in LSP but a proper way to do that would be for the metals server to announce the experimental capability for that feature in the capabilities that it returns from the initialize request. This is a standard practice for implementing experimental functionalities like this one. Once that is done, the LSP can enable such feature easily, based on the server capability.

ckipp01 commented 2 years ago

a proper way to do that would be for the metals server to announce the experimental capability for that feature in the capabilities that it returns from the initialize request

We can totally do this. @ayoub-benali feel free to shoot in a PR for this. You should be able to add the experimental stuff here https://github.com/scalameta/metals/blob/07a66ff10af4b63ee660e1ca432646a051268217/metals/src/main/scala/scala/meta/internal/metals/MetalsLanguageServer.scala#L786-L833