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

Added preferences for shell & shell arguments #7660

Merged
merged 1 commit into from
Apr 29, 2020

Conversation

owlJaeger
Copy link
Contributor

@owlJaeger owlJaeger commented Apr 23, 2020

Signed-off-by: Luca Jaeger owl.jaeger@gmail.com

What it does

Adds an easy way to control what shell or what shell arguments to use for the Terminal by adding preferences and prefer these over this.options.shellPath and this.options.shellArgs.

How to test

Add terminal.integrated.shell.{os} or terminal.integrated.shellArgs.{os} to the preferences.
Example:

{
  "terminal.integrated.shell.windows": "pwsh.exe",
  "terminal.integrated.shellArgs.windows": [
    "-nop"
  ]
}

I have pwsh.exe in my path env variable, but absolute paths work too.

Review checklist

Reminder for reviewers

@vince-fugnitto
Copy link
Member

@owlJaeger are these the preferences you are attempting to add:

Screen Shot 2020-04-23 at 8 30 02 AM

Screen Shot 2020-04-23 at 8 30 45 AM

@vince-fugnitto vince-fugnitto added preferences issues related to preferences terminal issues related to the terminal labels Apr 23, 2020
@owlJaeger
Copy link
Contributor Author

Yes, these are what I am looking for. I've updated this PR to include terminal.integrated.shell.${os} and terminal.integrated.shellArgs.${os} and not just one for all operating systems.

@akosyakov
Copy link
Member

Please make sure to align and test that they are compatible with VS Code if present.

@owlJaeger
Copy link
Contributor Author

Please make sure to align and test that they are compatible with VS Code if present.

I've tested it in the theia workspace and setting terminal.integrated.shell.windows to pwsh.exe inside .vscode while theia defaults to cmd.exe and the terminal started with powershell. So I see this as compatible? Is this what you meant?

@akosyakov
Copy link
Member

@owlJaeger that's good, each PR should pass https://github.com/eclipse-theia/theia/blob/master/doc/pull-requests.md#landing-pr someone will test and approve it

@owlJaeger
Copy link
Contributor Author

Looking forward to using it.

@owlJaeger
Copy link
Contributor Author

@akosyakov @vince-fugnitto I've now updated this PR. I am unsure about the naming of some stuff. I've tested it and it looks like it's fully functional on windows.

Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

My concerns were addressed. We need to test it though. It would be also good if @marechal-p look at the shell process. I think you changed it the last.

@owlJaeger owlJaeger force-pushed the master branch 2 times, most recently from 6e0a1a3 to 17e216d Compare April 28, 2020 17:18
Signed-off-by: Luca Jaeger <owl.jaeger@gmail.com>
Copy link
Member

@akosyakov akosyakov left a comment

Choose a reason for hiding this comment

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

I've tested on Linux with:

"terminal.integrated.shellArgs.linux": [
    "-c",
    "echo 'Hello World' && sleep 999999999"
  ]

and it worked well.

@vince-fugnitto @marechal-p Any concerns to merge it?

@vince-fugnitto
Copy link
Member

@vince-fugnitto @marechal-p Any concerns to merge it?

No I don't believe so :) I've restarted the build and it's all green.
Thank you very much for your contribution @owlJaeger!

Copy link
Contributor

@JPinkney JPinkney left a comment

Choose a reason for hiding this comment

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

Just tried it out on osx and everything is working!

@akosyakov
Copy link
Member

@vince-fugnitto @JPinkney thank you for double checking, merging 🚀

@akosyakov akosyakov merged commit 4c72757 into eclipse-theia:master Apr 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
preferences issues related to preferences terminal issues related to the terminal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants