-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Use new version of protocol to correctly serialize URIs #69453
Conversation
this seems like a good candidate for "merge without requirements" to unblock the C# release. |
@@ -37,6 +37,8 @@ void M() | |||
await using var testLspServer = await CreateTestLspServerAsync(markup, mutatingLspWorkspace); | |||
|
|||
var results = await RunGotoDefinitionAsync(testLspServer, testLspServer.GetLocations("caret").Single()); | |||
// Verify that as originally serialized, the URI had a file scheme. | |||
Assert.True(results.Single().Uri.OriginalString.StartsWith("file")); |
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 don't think this is necessarily correct assertion.
It is entirely correct to create Uri
instance using local path: new Uri(@"C:\a.cs")
. This gets internally translated to file:///C:/a.as
, so the AbsoluteUri
would be fine but OriginalString
would be C:\a.cs
.
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 the correct assertion for this particular test. The go to definition result is in a file with a path, so the json string the server serializes should be a file schemed URI. (e.g. file:///C:\a.cs)
The test client will then convert the server's json string response into LSP types (this is results
variable - its not the servers original csharp type, its the csharp type after the response has been deserialized back into csharp types on the client). So the original string it uses should be the file schemed URI that it got from the 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.
LGTM, other than the assertion.
I want to verify unit tests pass here at least, so will let it run |
No description provided.