Skip to content
This repository has been archived by the owner on Nov 18, 2022. It is now read-only.

Remove old rls.findImpls command #564

Merged
merged 2 commits into from
Apr 11, 2019

Conversation

Xanewok
Copy link
Member

@Xanewok Xanewok commented Apr 6, 2019

This was used before LSP has considered (and now stabilised)
a 'go to implementation' request. Handling this via LSP,
even at the cost of command disappearing from the context menu 1,
is preferred than using a global command. We'd have to maintain an
extra mapping and guess which RLS server is responsible for the file
for which a request has been made. This is hacky and not guaranteed
to be correct; meanwhile we get errors in multi-root workspaces
whenever we open more than one folder due to each instance trying to
register a global command under the same identifier.

The clean solution is to remove this extra command and use the standard
Go to Implementation (under Go > Go to Implementation [Ctrl + F12])
while we wait for the user configurable menus 2 to land.

Addresses #560 (doesn't fix unavailable commands in second directory)
Closes #437.

Xanewok added 2 commits April 7, 2019 00:43
This was used before LSP has considered (and now stabilised)
a 'go to implementation' request. Handling this via LSP,
even at the cost of command disappearing from the context menu [1],
is preferred than using a global command. We'd have to maintain an
extra mapping and guess which RLS server is responsible for the file
for which a request has been made. This is hacky and not guaranteed
to be correct; meanwhile we get errors in multi-root workspaces
whenever we open more than one folder due to each instance trying to
register a global command under the same identifier.

The clean solution is to remove this extra command and use the standard
Go to Implementation (under Go > Go to Implementation [Ctrl + F12])
while we wait for the user configurable menus [2] to land.

[1]: microsoft/vscode#54317
[2]: microsoft/vscode#9285
@xuchaoqian
Copy link

@Xanewok Hi. Would you like to merge this patch recently?

@Xanewok
Copy link
Member Author

Xanewok commented Apr 11, 2019

I'd love to but I'm afraid that losing the menu entry might cause an uproar, similar to dropping the cargo run task - #557 (comment)

@Xanewok
Copy link
Member Author

Xanewok commented Apr 11, 2019

Let's merge this - hopefully there won't be angry users this time 😁

@Xanewok Xanewok merged commit 69107f6 into rust-lang:master Apr 11, 2019
@Xanewok Xanewok deleted the remove-custom-impls-command branch April 11, 2019 18:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"find implementations failed: Error: An unknown error occurred"
2 participants