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
Add PS Core in select default shell on windows#72425 #72689
Add PS Core in select default shell on windows#72425 #72689
Conversation
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.
@sbatten please review the usage of vscode-windows-registry 🙂
@@ -131,9 +154,12 @@ export class TerminalService extends BrowserTerminalService implements ITerminal | |||
useWSLexe = true; | |||
} | |||
|
|||
const Registry = await import('vscode-windows-registry'); |
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.
Let's move the await into _getShellPathFromRegistry
and make that function async as well
* @param shellName The shell name to get the executable file path | ||
* @returns The executable file path of shell or `'ShellNotFound'` | ||
*/ | ||
private _getShellPathFromRegistry(Registry: typeof import('vscode-windows-registry'), shellName: string): string { |
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.
I think this function should be a little more targetted to just pwsh for now and then return either []
or [ 'path' ]
let shellPath; | ||
|
||
try { | ||
shellPath = Registry.GetStringRegKey('HKEY_LOCAL_MACHINE', `SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\App Paths\\${shellName}.exe`, ''); |
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.
This could be simplified:
try {
return (grab from reg);
} catch () {
}
return [];
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.
Registry.GetStringRegKey()
function returns string
or undefined
.
https://github.com/Microsoft/vscode/blob/3a12b7ac2efd2f1f01a646ef5272313ad50bf618/src/typings/vscode-windows-registry.d.ts#L8
Therefore, this function needs a logic to convert from undefined
to []
.
But I referred your review when I fixed the code.
Thanks.
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.
I realized that using !
makes the logic unnecessary. So I deleted the logic.
sorry for the trouble!
const Registry = await import('vscode-windows-registry'); | ||
|
||
try { | ||
return [Registry.GetStringRegKey('HKEY_LOCAL_MACHINE', `SOFTWARE\\Microsoft\\Windows\\CurrentVersion\\App Paths\\${shellName}.exe`, '')!]; |
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.
While this does compile, you'll still want to put it in a variable and check for undefined just in case as !
tells the typescript compiler to don't type check this making it less safe. If the key didn't exist (for some reason) this would return [undefined] which could break whatever is consuming the function.
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.
Thank you for review,
That was a failure due to my lack of understanding.
I fixed this code.
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.
Looks great! I'll verify and merge in the May release 🙂
Issue number
#72425
Description
This PR adds pwsh(PowerShell Core) in Select Default Terminal UI when VSCode is operated on Windows.
A file path of pwsh is got from registry.
Screenshot
Here is a screenshot of a pattern with pwsh installed with a custom path.
Note
I wrote the below in added a function.
We will find
ShellNotFound
in a shell path of Select Default Terminal UI iffile://ShellNotFound
exists and added a function can't get a shell path.Because I can not write the below by #72601.
I suggest to change from
'ShellNotFound'
to''
if #72601 is fixed.