Skip to content
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

Merged
merged 23 commits into from Jul 6, 2021

Conversation

333fred
Copy link
Member

@333fred 333fred commented May 30, 2021

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.

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.
@333fred 333fred changed the title gotodef v2 Adds support for the V2 GotoDefinition endpoint May 30, 2021
@333fred
Copy link
Member Author

333fred commented May 30, 2021

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.

@333fred 333fred requested a review from JoeRobich May 30, 2021 06:33
@333fred
Copy link
Member Author

333fred commented Jun 1, 2021

Now additionally needs OmniSharp/omnisharp-roslyn#2170 to be merged, as it adds support for source-generated results in go-to-definition.

@333fred
Copy link
Member Author

333fred commented Jun 4, 2021

@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.

Copy link
Contributor

@filipw filipw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM thank you!

@333fred
Copy link
Member Author

333fred commented Jun 19, 2021

@JoeRobich can you take a look at the changelog descriptions?

Copy link
Member

@JoeRobich JoeRobich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@333fred 333fred enabled auto-merge June 19, 2021 01:35
@333fred 333fred disabled auto-merge June 19, 2021 01:35
@333fred 333fred enabled auto-merge (rebase) June 19, 2021 01:35
@333fred 333fred disabled auto-merge June 19, 2021 03:44
@333fred
Copy link
Member Author

333fred commented Jun 20, 2021

@JoeRobich do you have any ideas here? I'm reasonably confident that my code isn't touching anything related to diagnostics...

@JoeRobich
Copy link
Member

@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

@333fred
Copy link
Member Author

333fred commented Jul 4, 2021

@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.

Copy link
Member

@JoeRobich JoeRobich left a 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.

@JoeRobich JoeRobich merged commit 5e48576 into dotnet:master Jul 6, 2021
@333fred 333fred deleted the gotodef-v2 branch July 6, 2021 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unskip definitionProvider test
3 participants