Implement support for multi folder workspaces #355
Conversation
sadesyllas
force-pushed
the
workspace-support
branch
from
922ba75
to
a29d2eb
Compare
package.json
Outdated
| @@ -2,7 +2,7 @@ | |||
| "name": "vscode-rust", | |||
| "displayName": "Rust", | |||
| "description": "Rust language integration for VSCode", | |||
| "version": "0.4.2", | |||
There was a problem hiding this comment.
Could you explain why you want to increase the version of the extension?
There was a problem hiding this comment.
Your are right, I shouldn't have done so. I have reverted the change.
src/components/cargo/CargoManager.ts
Outdated
| if (!name || name.length === 0) { | ||
| this._currentWorkingDirectoryManager.cwd().then(cwd => { | ||
| if (!cwd) { | ||
| window.showErrorMessage('Current document not in the workspace'); |
There was a problem hiding this comment.
I think it should be "Current document is not in the workspace", shouldn't it?
There was a problem hiding this comment.
I believe that it was already written this way. Apart from refactoring that part I did not change the string's content. But now that you have mentioned it, I just amended it. 😃
| @@ -93,7 +102,7 @@ export class CurrentWorkingDirectoryManager { | |||
|
|
|||
| return this.checkPathExists(pathToCargoTomlInPreviousCwd).then<string>(exists => { | |||
| if (exists) { | |||
| return Promise.resolve(this.rememberedCwd); | |||
| return Promise.resolve(<string>this.rememberedCwd); | |||
There was a problem hiding this comment.
This was done so that the linter would stop nagging since there, we say that we will eventually return a Promise<string>, but when resolving with Promise.resolve(this.rememberedCwd), the final type of the promise that we are returning is actually Promise<string|undefined>.
Thus, to get rid of the inconsistency I wrote it explicitly since at that point we will not return undefined.
There was a problem hiding this comment.
I believe it could be spelled like this.rememberedCwd!, just to drop the undefined type. That way you still propagate the string type instead of asserting it.
There was a problem hiding this comment.
@Frizi, you are correct, I changed that part to this.rememberedCwd! as per your comment.
sadesyllas
force-pushed
the
workspace-support
branch
from
a29d2eb
to
b7daf81
Compare
|
@KalitaAlexey, did you have time to check the PR? |
sadesyllas
force-pushed
the
workspace-support
branch
from
b7daf81
to
d07f6ba
Compare
This is an attempt to support the new multi folder workspaces feature.
I have completely changed my initial approach as described in #349 and instead opted for a far simpler change.
The main change is how
CurrentWorkingDirectoryManager.cwd()works.I can't say that I am aware of every aspect of this project and unfortunately I did not have the time to test absolutely everything after my changes but simply removing any use of
rootPathand using the new workspaces API seems to be working for me without any problems during my usual, yet simple, workflow.I the PR has a correct basis please provide feedback to change/correct any mistakes that I may have made.