-
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
fix #7269: complete support vscode.workspace.fs API #7908
Conversation
6c9058c
to
f207756
Compare
fc13654
to
b80c7a9
Compare
d8151fe
to
4009dcc
Compare
446e385
to
aef2b6e
Compare
@marcdumais-work Is it mandatory for us to wait till CQs where we know that code is under MIT to be approved by the foundation? I remember we discussed that we could merge it and do occasional checks of the whole VS Code? It takes too long and no response from them. btw it looks like there are already solutions in the wild based on this PR: https://twitter.com/plotlygraphs/status/1281672032955047938 Notebooks cannot work without this PR. |
Yes, I think we need to wait for CQs about copied code, unless given permission to merge while they finish their investigation (so-called "parallel IP*"). The main vscode repo's license is indeed MIT, but there are bits that are under different licenses. So depending what exactly we copy, it could have an impact. I've pinged the PMC on the CQ, asking to be granted parallel IP.
Yes, that was a proposal by the foundation, for them to deep-analyse whole versions of vscode, making them pre-approved, for us to use/fork (in theory). I think there was an assumption that this would not need to happen too often (i.e. not every vscode release) and that we would/could use these few pre-approved versions most of the time. So maybe more a way to save work for the Foundation, rather than make it more efficient for us :) In this PR it looks like you copy from the as-of-yet unreleased (at CQ creation time) (*): from the Eclipse Project Handbook:
|
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.
Looking forward to seeing this PR get merged. I have mostly questions, with a few suggestions sprinkled in.
@tsmaeder thank your for the review! I will work through comments next week. CQ was approved btw, so maybe we can land it end next week. |
5cf3072
to
f408df3
Compare
@tsmaeder I've addressed your comments. Could you have a look please? @eclipse-theia/ecd-theia-committers CQ is approved, I would like to merge it after next release, i.e. this Thursday if no objections... should not forget to bump up the version in CHANGELOG and rebase once more after the release |
f408df3
to
2698007
Compare
@akosyakov all my comments are resolved. |
2698007
to
87c7840
Compare
Signed-off-by: Anton Kosyakov <[email protected]>
87c7840
to
a8d9c09
Compare
Does this update change https://github.com/eclipse-theia/theia/wiki/Coding-Guidelines#uripath? |
@yunuscukran updated guidelines |
What it does
This PR replaces our filesystem implementation with VS Code one to provide first class support for different fs providers and features like dealing with encoding, cross provider operations, support working copy modifications and so on.
getCurrentUserHome
toEnvVariableServer
getDrives
toEnvVariableServer
vscode-userdata
andvscode-remote
fs providers delegating touserstorage
andfile
schemesFileSystemNodeOptions
? is it not used in practice and there are user preferences to control trash and encodingFileService
FileService
DiskFileSystemProvider
should supportuseTrash
thenOut of scope:
How to test
Select for Compare
andCompare with Selected
commands should workfiles.watcherExclude
from watchingGit Diff: Compare with...
command should workGit: Clone
command should workGit: Discard All Changes
command should workSCM: Change Repository...
command should workFind in Folder
command should work from the navigatorcwd
,workspaceFolder
,workspaceRoot
, andfile
variables should be resolved properlyReview checklist
Reminder for reviewers