- Notifications
You must be signed in to change notification settings - Fork 110
Added proposed features from lsp 3.16 #230
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
Conversation
If there only was a |
…at the supported set can be defined by the client
… like enums but can be extended with other values based on clients
| @mholo65 maybe we should make one! Until then for now it'll at least point out that these bits may change or many not work without extra work. |
Codecov Report
@@ Coverage Diff @@ ## master #230 +/- ## ========================================== + Coverage 43.94% 44.60% +0.65% ========================================== Files 361 360 -1 Lines 8060 8280 +220 Branches 987 1023 +36 ========================================== + Hits 3542 3693 +151 - Misses 4226 4286 +60 - Partials 292 301 +9
Continue to review full report at Codecov.
|
| | ||
| /// <summary> | ||
| /// The client supports tags on `SymbolInformation`.Tags are supported on | ||
| /// `DocumentSymbol` if `hierarchicalDocumentSymbolSupport` is set tot true. |
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.
| /// `DocumentSymbol` if `hierarchicalDocumentSymbolSupport` is set tot true. | |
| /// `DocumentSymbol` if `hierarchicalDocumentSymbolSupport` is set to true. |
| return builder.Commit().GetSemanticTokensEdits(); | ||
| } | ||
| | ||
| // TODO: Is this correct? |
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.
is it? :)
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 think that was from before I had added the helpers for tokenization. At this point I think it's good based on my testing, and if it isn't I made it virtual for a reason 😄
src/Protocol/DocumentUri.cs Outdated
| /// <summary> | ||
| /// Helper methods for working with LSP document URIs. | ||
| /// </summary> | ||
| public static class DocumentUri |
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.
This is an interesting class. Would it handle VS Code's conflicting Uri type (aka the fact that C: is encoded to C%3A)
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.
We probably should add that here with some unit tests to make sure.
This was brought up from the client I think.
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.
In those tests, can you also include tests of Chinese characters or Cyrillic or something? Those don't always play nicely with the Uri type...
| /// Character offset on a line in a document (zero-based). | ||
| /// </summary> | ||
| public long Character { get; set; } | ||
| public int Character { get; set; } |
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.
any reason for changing all the longs to ints?
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.
To make it easier to deal with the math when generating semantic tokens, otherwise a lot of potential conversions can happen. I can try and roll it back if it is needed for something. I dunno how many files have 2GB characters on a single line.
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.
PowerShell uses ints so I'll be happy to remove a bunch of (int)s all over the place :)
| I assume that this should "just work" with language servers that only support 3.15 handlers and also clients that only support 3.15-styled messages. |
Co-authored-by: Tyler James Leonhardt <tylerl0706@gmail.com>
| Yeah, the goal here is existing servers will work, and if implement the new features and run into an editor that also implements the new features, things will "just work". |
…vscode %3A 'feature'
…anguage-server-protocol into feature/proposed
src/JsonRpc/OutputHandler.cs Outdated
| { | ||
| _outputHandler.LogTrace(e, "Could not write to output handler, perhaps serialization failed?"); | ||
| _outputIsFinished.TrySetException(e); | ||
| // _thread.Abort(e); |
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.
commented out code?
| <PackageDescription>Primitives for working with JsonRpc. This library is used as the base for communication with language servers</PackageDescription> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <PackageReference Include="System.Collections.Immutable" /> |
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.
Curious what this is good for over IReadOnly* types.
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 grabbed it for ImmutableArray mainly because it "does the right thing" when it comes to creating new and transformed arrays.
I used this so I don't have to think too hard about copying arrays around. This is because SemanticTokensDocument keeps a copy of the previous tokens, and the newly computed tokens, so that it can handle computing the diff between the two.
I was porting the builder from https://github.com/microsoft/vscode-languageserver-node/blob/master/server/src/semanticTokens.proposed.ts#L45 😄
…on from file system like paths (windows and unix), uri strings (scheme://...) and Uri. In addition it manages the funny serialization that VSCode does in regards to : on windows file systems.
This adds the current proposals for the next version of the language server protocol.
I'm introducing these with
[Obsolete]attribute to indicate that they are preview and they may change in the future. When 3.16 comes out we can simply remove the attributes and move the namespaces.I got this working with a nice disco look in regards to semantic tokens 😄
