Make FileSystemCommands.UPLOAD to return FileUploadResult

[tomer-epstein]

Signed-off-by: Tomer Epstein tomer.epstein@sap.com

What it does

Make FileSystemCommands.UPLOAD to return FileUploadResult

How to test

git clone https://github.com/tomer-epstein/vscode-theia-tomer.git
cd vscode-theia-tomer
npm i && npm run package
copy vscode-theia-tomer-0.0.1.vsix to plugins folder
Right click on a folder in the Explorer, choose 'Open Theia Upload Dialog'.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[tomer-epstein]

Adding reference to Refactor upload functionality

[vince-fugnitto]

@tomer-epstein can you clarify what the pull-request is attempting to fix and why, it is not clear with the current provided description. In addition, it would help to provide a sample vscode extension for reviewers to test with (it may also help understanding why the changes are necessary).

[tomer-epstein]

@vince-fugnitto , Sure as usual I'll provide a sample vscode ext to test the change.
We would like to perform some tasks (like unzip) on the uploaded files, for this we need the command to return the upload result.

[amiramw]

Code LGTM, also tested successfully.

@vince-fugnitto do you think we should update changelog for this command new return value?

[vince-fugnitto]

@vince-fugnitto do you think we should update changelog for this command new return value?

@amiramw I do not believe so since the original return value is void and adding a new value would not be a breaking change.
I do add changes when I prepare the changelog for a release :)

[vince-fugnitto]

I confirmed that the changes work correctly 👍

• verified the code
• verified that the plugin works correctly when uploading the file
• verified both on the browser and electron targets:
  • browser: successfully uploads
  • electron: the upload functionality is disabled on electron correctly resulting in undefined

[tomer-epstein]

@vince-fugnitto may you merge? I don't have perms.

[vince-fugnitto]

@vince-fugnitto may you merge? I don't have perms.

@tomer-epstein I'd love to, but we decided as a community in our last dev meeting that we should not merge any new pull-requests (no matter how small) until we perform the release (this Thursday) or we have CI running again. We are currently working on bringing CI back by re-implementing it using GitHub Actions.

[vince-fugnitto]

@tomer-epstein do you mind rebasing the pull-request against master (so we can run ci again using github actions), I'll merge shortly afterwards :)

[tomer-epstein]

@vince-fugnitto you may merge.