Skip to content
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 add missing await on workspace edits file create #6851

Merged
merged 1 commit into from
Jan 13, 2020

Conversation

amiramw
Copy link
Member

@amiramw amiramw commented Jan 8, 2020

Signed-off-by: Amiram Wingarten amiram.wingarten@sap.com

What it does

Fixes a missing await for file creation done as part of workspace edits.

How to test

VS Code extension containing code like the below will create an empty file as the promise does not wait for the file to be created.

let we = new vscode.WorkspaceEdit();
let pathUri = vscode.Uri.file(path);
we.createFile(pathUri);
vscode.workspace.applyEdit(we).then(function() {
 writeFileSync(tasksPathUri.fsPath, "some content");
});

Review checklist

Reminder for reviewers

Signed-off-by: Amiram Wingarten <amiram.wingarten@sap.com>
@akosyakov akosyakov requested a review from kittaakos January 9, 2020 02:04
@akosyakov akosyakov added the workspace issues related to the workspace label Jan 9, 2020
@akosyakov akosyakov added the vscode issues related to VSCode compatibility label Jan 9, 2020
Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amiramw I tried to create a VS Code extension which performs the same behavior as you described in the pull request, however the extension worked correctly in master. Perhaps you can share your own extension so I can more easily test?

My extension is the following:
https://github.com/vince-fugnitto/theia-test-ext/releases/download/1.0.0/test-ext-0.0.1.vsix

@amiramw
Copy link
Member Author

amiramw commented Jan 12, 2020

@vince-fugnitto
The issue occurs when the creation is to a file with its parent folder, both are new.
The problem reproduces when I change your path calculation to:

vscode.Uri.file(workspacePath.uri.fsPath + '/newdir/test.txt');

Copy link
Member

@vince-fugnitto vince-fugnitto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@amiramw I updated my example extension and noticed the bug on master.
Thanks to your update I see the correct behavior, the folder and file are created with the correct content 👍

@vince-fugnitto
Copy link
Member

@amiramw do you need help with the merge?

@amiramw
Copy link
Member Author

amiramw commented Jan 13, 2020

yes. I don't have merge permissions.

@vince-fugnitto vince-fugnitto merged commit b631dfc into eclipse-theia:master Jan 13, 2020
@amiramw amiramw deleted the await branch January 13, 2020 13:10
@akosyakov
Copy link
Member

@marcdumais-work Do you know why @amiramw don't have permissions? Here is a committer.

@marcdumais-work
Copy link
Contributor

@marcdumais-work Do you know why @amiramw don't have permissions? Here is a committer.

I do not see him as part of the org's members. When the org was crated, each committer should have received an invite, that one must accept before being granted effective write permission.

But we had at least one case where a committer was not "synced" correctly and so left out. Maybe this is another instance of this? I would suggest opening a bugzilla under "community" -> "github" and ask to check why @amiramw is not part of the membership.

@amiramw
Copy link
Member Author

amiramw commented Jan 14, 2020

@marcdumais-work should I open it? where exactly?

@marcdumais-work
Copy link
Contributor

@marcdumais-work should I open it? where exactly?

Yes, that would be the most efficient since you'll need to at least validate it works in the end.

Start here and select "github" as component.

@amiramw
Copy link
Member Author

amiramw commented Jan 15, 2020

Apperently I didn't have the correct github id in my eclipse profile. Seems fine now.

Thanks @marcdumais-work @akosyakov !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vscode issues related to VSCode compatibility workspace issues related to the workspace
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants