-
Notifications
You must be signed in to change notification settings - Fork 677
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
Adds support for the V2 GotoDefinition endpoint #4581
Conversation
The V2 endpoint can return multiple definitions, allowing gotodefinition on a partial type to fully enumerate all locations the user would want to go to. While I was here, I unskipped a test that was otherwise passing. Closes dotnet#3458.
Note: the new test added for partials won't pass until OmniSharp/omnisharp-roslyn#2168 is merged and we consume the new version in this repo. |
…g those files when switching to the file.
Now additionally needs OmniSharp/omnisharp-roslyn#2170 to be merged, as it adds support for source-generated results in go-to-definition. |
@JoeRobich @filipw @david-driscoll would be good to get some eyes on the implementation of this, particularly the new virtual document source for source generators. |
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 thank you!
@JoeRobich can you take a look at the changelog descriptions? |
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 good!
@JoeRobich do you have any ideas here? I'm reasonably confident that my code isn't touching anything related to diagnostics... |
@333fred This is what the diagnostic integration tests do when they have to set a value like decompilation which requires a restart https://github.com/OmniSharp/omnisharp-vscode/blob/master/test/integrationTests/diagnostics.integration.test.ts#L161 |
…ecific source generator test.
@JoeRobich can you take another look? I gave up on having the generator test in the same solution as the rest of them, and split it out into a separate solution. |
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 good! I'll follow up and remove all the "These tests don't run" comments as they are unnecessary and stale.
The V2 endpoint can return multiple definitions, allowing gotodefinition on a partial type to fully enumerate all locations the user would want to go to.
While I was here, I unskipped a test that was otherwise passing. Closes #3458.