-
Notifications
You must be signed in to change notification settings - Fork 324
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
Proposed extensions: textDocument/selectionRange #441
Conversation
Status: I've got the code to compile, but I haven't actually tested it with my server. Not sure how all proposed bits, in the VS Code and in the lsp client library, should fit together. |
c40a2a4
to
222a999
Compare
Status update: I've successfully (after fiddling with npm quite a bit) built my extension with |
The server impl is read, and server and client successfully work together: rust-lang/rust-analyzer@fffa069! The only quirk is that current insider's does not have the latest version of proposed API (it uses Range instead of SelectionRange), so I had to tweak client library a bit to work with old API. |
Great to see that you got it working. Please ping when the API and implementation is finalized and then we can merge it in. |
Sure! I believe this is currently blocked on VS Code side (some questions there). @jrieken could you ping me once VS Code side is "finalized" (I understand that this is provisional API, hence quotes). |
@matklad So far we are pretty happy with our proposal. Tho we haven't implemented any useful provider yet ;-) The idea is to that in the next weeks and to validate our design with that. Having said that, we will likely keep the API in proposed for another month. @dbaeumer Isn't an option for LSP proposal? Have this as proposed protocol first and validate it with some of our own language brains, like JSON and CSS? |
222a999
to
165c80c
Compare
165c80c
to
54480ec
Compare
I've rebased and switched to the new API, but I am having hard-time actually testing my client with this code (I had success the time before this, but I can't reproduce it). Is there anything more to using my fork of
npm gives me some weird errors about extraneous packages :( whyyyyy |
@matklad I usually use links. (e.g. I go into the node_modules folder delete |
@dbaeumer that did the trick, thanks! I've verified that all the pieces work together, including my server. |
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.
I took a pass at it and it looks good to me too. Thank you for your contribution, @matklad!
I will merge this in and ship a next version as soon as we released the next VS Code stable version which contains the necessary proposed API. |
fyi - after implementing some provider and actually using them we got the feeling of going 180 of the single position vs multi position. The VS Code API will very likely be changed to accept multiple positions and return an array of ranges |
I merged the PR in and made new next version: |
@jrieken I am a little confused about VS Code state here. Are you switching to multi-position in the end? The current proposed API is still single-position. |
Yeah, switching to multiple positions is the current plan. Not sure why @dbaeumer already merged this... |
@matklad @jrieken It's probably @dbaeumer feels that the LSP API itself will not change so he would like adopters to start trying out and using the LSP API. Or do we now anticipate the LSP API changing also? |
This got released as a next version (*-next.1) to get feedback on the LSP level as well. Normal adopters will not see that version and we are free to change the LSP API to multiple positions which I suggest to do. |
In addition the feature is in proposed state and users need to opt in using it. |
@dbaeumer FYI, the vscode API changed again:
|
@matklad I noticed that as well. Are you willing to work on a adoption PR. We can merge it in as soon as we have another stabled release with the new API. |
@dbaeumer yeah, I'll most likely handle this withing a week |
done: #474 (review) |
Implementation for microsoft/language-server-protocol#613