-
Notifications
You must be signed in to change notification settings - Fork 11
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
Send range with textDocument/hover #45
Conversation
This is needed to handle scalameta/metals#3060 It should be handle by LSP package once microsoft/language-server-protocol#377 is resolved
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 But before adding something like that, @rwols should also state his opinion. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks much better than the workaround in VS Code!
LGTM from overall code perspective, but I do not know enough about the Sublime API to see specific possible issues.
This an alternative solution to scalameta/metals-sublime#45 The current implementation is just a workaround until microsoft/language-server-protocol#377 is part of the LSP spec
This an alternative solution to scalameta/metals-sublime#45 The current implementation is just a workaround until microsoft/language-server-protocol#377 is part of the LSP spec
This a better alternative to scalameta/metals-sublime#45 and this implementation is just a workaround until microsoft/language-server-protocol#377 is part of the LSP spec
The downside of this approach is that it creates a new command that very similar to the base |
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. |
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. |
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 |
This is needed to handle scalameta/metals#3060
It should be handle by LSP package once microsoft/language-server-protocol#377
is resolved
@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:
closes #44