Discover new PowerShell installations, fix startup issue with Windows PowerShell #2238
Conversation
|
@rjmholt can you make a goal of this to be that someone could copy this code and use it in their own project? The Azure Data Studio folks want this. |
rjmholt
changed the title
|
@TylerLeonhardt It looks like we need to merge PowerShell/PowerShellEditorServices#1066 to get tests to proceed |
|
Yep - that was needed |
Co-Authored-By: Tyler James Leonhardt <[email protected]>
| : getDefaultPowerShellPath(this.platformDetails, this.sessionSettings.useX86Host); | ||
| // No need to resolve this path, since the finder guarantees its existence | ||
| const firstPowerShell = this.powershellExeFinder.getFirstAvailablePowerShellInstallation(); | ||
| return firstPowerShell && firstPowerShell.exePath || null; |
There was a problem hiding this comment.
The && and throwing me off here. Does that return firstPowerShell if firstPowerShell and firstPowerShell.exePath are truthy - else null?
There was a problem hiding this comment.
&& returns either the falsey argument, or if both are truthy, the rightmost.
So "Hello" && "there" evaluates to "there'.
So a common idiom I've seen is to use it like obj?.property
There was a problem hiding this comment.
oh I see. How about ternary instead? Maybe a little cleaner and more well known?
| return firstPowerShell && firstPowerShell.exePath || null; | |
| return firstPowerShell ? firstPowerShell.exePath : null; |
There was a problem hiding this comment.
Not as safe: if exePath is not defined, then the first will give a known default when the second does not
| suite("Platform module", () => { | ||
| let tempEnv: NodeJS.ProcessEnv; | ||
|
|
||
| suite("PlatformDetails", () => { |
There was a problem hiding this comment.
Thanks for rewriting these!
Co-Authored-By: Tyler James Leonhardt <[email protected]>
|
@TylerLeonhardt ready for review |
|
as soon as CI passes feel free to merge. |
PR Summary
Fixes #2217.
Gets rid of the default PowerShell and available PowerShell list logic in favour of a lazily generated list of PowerShells.
This resolves the issue where the PowerShell extension wouldn't start if PowerShell Core is not installed.
It also adds discovery of the PowerShell MSIX installation.
Discovery of the PowerShell .NET Core Global Tool has been added but is commented out, since a bug in this release persists that prevents passing command arguments needed to start the extension.
TODO: Add more testsTests refactored and added.
PR Checklist
Note: Tick the boxes below that apply to this pull request by putting an
xbetween the square brackets.Please mark anything not applicable to this PR
NA.WIP:to the beginning of the title and remove the prefix when the PR is ready