-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[workspace] workspace.supportMultiRootWorkspace
is enabled by default
#6089
Conversation
6f0ebf4
to
de829a6
Compare
@marcdumais-work @vince-fugnitto do you know when @elaihau is available next time? Maybe someone else can check whether all clients are respecting multi-root workspaces, and not simply fallback to the first root. I know for sure that for LSP support of multi-roots #5901 is required. |
@@ -34,7 +34,7 @@ export const workspacePreferenceSchema: PreferenceSchema = { | |||
'workspace.supportMultiRootWorkspace': { | |||
description: 'Enable the multi-root workspace support to test this feature internally', |
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 think the description should be changed also.
I know that @elaihau will be in the Montreal offices on Friday, but I'm not sure when else during the week he will be working remotely. |
I just assigned @elaihau because he was proposed. Happy to go with any review :-) |
de829a6
to
2b63bdf
Compare
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.
Minor comments to adjust before merging
Works fine
CHANGELOG.md
Outdated
@@ -716,3 +716,4 @@ Breaking changes: | |||
- [git] added `git sync` and `git publish` actions | |||
- [navigator] added the ability to toggle hidden files in the navigator | |||
- `jdt.ls` download on postinstall | |||
- `workspace.supportMultiRootWorkspace` is enabled by default |
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 line should not be under the version v.0.3.11, but under the next release on top of the file (v0.10.1 or v0.11)
2b63bdf
to
4399925
Compare
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
@akosyakov could you clarify? I use multi-root workspace with a go mono repo. Because the Go language server (gopls) wants every module to be in a root. In that case LSP seems to work fine. |
@svenefftinge It affects only if a language server supports multiple workspace roots, we were reporting only first. It also does not affect VS Code extensions, only native extensions. |
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've reviewed all usages of roots and seems like all clients are capable to handle multiple roots.
Also tested a bit with single root and everything behaves well too.
Fixes #6088 Signed-off-by: Sven Efftinge <[email protected]>
4399925
to
7a84d9d
Compare
I'm wondering if there's actually a need for the preference. In the past the preference was used to turn off the feature since it was experimental. Now that the preference is enabled I'm wondering if we should simply remove it. Multi-root workspace support would then only be enabled the moment a user adds a new root. |
What it does
Enables
workspace.supportMultiRootWorkspace
by default.Fixes #6088
How to test
Start theia and see how multiroot workspaces are enabled.
E.g.
Save workspace as ...
menu item is enabled.Review checklist
Reminder for reviewers