Support VSCode workspaces #333
Conversation
There was a problem hiding this comment.
This looks good! It took some time to initially understand the changes (left some comments) but that might be just me coming back after a break.
This feature looks like something that might finally push us to later dig into VSCode testing and hopefully test some of the related functionality, if it's possible!
| if (workspaces.has(folder.uri.toString())) { | ||
| continue; | ||
| } | ||
| for (const f of fs.readdirSync(folder.uri.fsPath)) { |
There was a problem hiding this comment.
I understand this checks if the added workspace, which is not contained in one of the existing workspace folders, contains a Cargo.toml file to decide whether to run the server - maybe we more idiomatic would be to use the newly added workspace.findFiles with RelativePattern to determine whether it contains a Cargo.toml file?
There was a problem hiding this comment.
I don't think this works unfortunately, because at this point we're checking whether we should create a workspace and to use RelativePattern you need a workspace already created.
src/extension.ts
Outdated
| // We run one RLS and one corresponding language client per workspace folder | ||
| // (VSCode workspace, not Cargo workspace). This class contains all the per-client | ||
| // and per-workspace stuff. | ||
| class Workspace { |
There was a problem hiding this comment.
It took me a while to understand what is referred to by workspace(s) until I got to the definition here - I know naming is hard and this might be too generic, but maybe something along the lines of ServerFolderContext might be better? Also the workspace namespace refers to the single VSCode workspace that consists of folders, so having multiple Workspaces coupled with VSCode workspace folders seems somewhat confusing here.
There was a problem hiding this comment.
Hmm, yeah, agree Workspace is a bit hard to understand. I've gone for ClientWorkspace I don't think it is much better, but I think it is important to note that this is about the language client, rather than the server and about vscode workspaces.
| } | ||
|
|
||
| let lc: LanguageClient; | ||
| function didChangeWorkspaceFolders(e: WorkspaceFoldersChangeEvent, context: ExtensionContext): void { | ||
| _sortedWorkspaceFolders = undefined; |
There was a problem hiding this comment.
minor nit: I think it'd be good to highlight the fact that getOuterMostWorkspaceFolder caches the result and reuses it; at a glance it's hard to spot that calling the function reuses a global cache here
|
Thanks for the review! |
r? @Xanewok
Implements the 'server per folder' approach described in https://github.com/Microsoft/vscode/wiki/Extension-Authoring:-Adopting-Multi-Root-Workspace-APIs#language-client--language-server