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 vscode.window.createTerminal with cwd #9140

Merged
merged 1 commit into from
Mar 2, 2021

Conversation

veredcon
Copy link
Contributor

@veredcon veredcon commented Mar 1, 2021

Signed-off-by: Vered Constantin [email protected]

Fix #7586

With this PR, creating terminal with cwd option will make sure the cwd is a string as it should be.
(As specified in TerminalWidgetOptions.cwd )

What it does

It makes the createTerminal API more robust even in case the cwd passed by extensions is URI (like happens with vscode-maven extension). VScode still works fine even when extension is passing URI as cwd to this API.

How to test

Explained in #7586:

  1. Install vscode-maven, all versions starting from https://open-vsx.org/api/vscjava/vscode-maven/0.21.2/file/vscjava.vscode-maven-0.21.2.vsix
  2. Click on the project context menu: clean/install etc.

Expected Result:
A terminal should be opened with the corresponding command being executed in the terminal

Review checklist

Reminder for reviewers

Copy link
Contributor

@kittaakos kittaakos left a comment

Choose a reason for hiding this comment

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

It looks good to me. Thank you for the patch, @veredcon 👍


@vince-fugnitto, could you please proceed with the merge if you're also fine with the PR? Thank you!

@veredcon
Copy link
Contributor Author

veredcon commented Mar 2, 2021

@kittaakos thanks for the quick review!
seems like some test not related to this change fails in ubuntu?... I noticed this in the logs:
log-level-cli-contribution
@theia/core: should read json config file:
@theia/core: Error: Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves. (/home/runner/work/theia/theia/packages/core/lib/node/logger-cli-contribution.spec.js)

are you familiar with this? I cannot also retrigger

@vince-fugnitto vince-fugnitto added terminal issues related to the terminal vscode issues related to VSCode compatibility labels Mar 2, 2021
@vince-fugnitto
Copy link
Member

@veredcon I believe the CI was failing since it may not be based on the latest master, but it should not be an issue (I restarted the build anyways).

@vince-fugnitto vince-fugnitto merged commit 1297c63 into eclipse-theia:master Mar 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
terminal issues related to the terminal vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MAVEN from open-vsx-registry version 0.21.2 does not work for items found in the context menu
3 participants