Implementation of additional exe paths #1270

rkeithhill · 2018-04-16T01:37:06Z

BTW this does change the session menu to represent the "current" session with the same name as the session name as it appears when it is not selected. By doing it this way, it is easier to find the "versionName" field for the powershell.powerShellDefaultVersion setting.

Here's the list before:

And after:

I put in some extra logic to only display PS Cores where pwsh.exe is present instead of just the directory as it appears the PS Core uninstaller leaves the empty dir on the file system. :-(

As for the extra info (edition / specific version number), you can easily get that by executing $PSVersionTable in the PSIC.

Here is the setting for adding other PowerShell exes:

    "powershell.powerShellAdditionalExePaths": [
        {
          "versionName": "PowerShell Core Build - Debug",
          "exePath": "C:\\Users\\Keith\\GitHub\\rkeithhill\\PowerShell\\debug\\pwsh.exe"
        }
    ]

The above is a User setting only. The following is both a User and Workspace setting:

    "powershell.powerShellDefaultVersion": "PowerShell Core Build - Debug"

Fix #1243

TylerLeonhardt

LGTM Keith! Just had a few questions 😄

TylerLeonhardt · 2018-04-16T22:34:19Z

src/session.ts

+        // If powershell.powerShellDefaultVersion specified, attempt to find the PowerShell exe path
+        // of the version specified by the setting.
+        if ((this.sessionStatus === SessionStatus.NeverStarted) && this.sessionSettings.powerShellDefaultVersion) {
+            const powerShellExePaths = getAvailablePowerShellExes(this.platformDetails, this.sessionSettings);


I think I'm going crazy but where is getAvailablePowerShellExes defined? I see it's in platform.ts but not it session.ts. How is being called here?

I trust it works, but I'm just curious.

It's imported from platform.ts on line 25.

TylerLeonhardt · 2018-04-16T22:35:37Z

src/session.ts

+        powerShellExePath =
+            (powerShellExePath ||
+             this.sessionSettings.powerShellExePath ||
+             this.sessionSettings.developer.powerShellExePath ||


this is for backcompat?

The check for this.sessionSettings.developer.powerShellExePath is for back compat. For 2.0, I say we remove the setting and this fallback.

I'm assuming it's a fallback? Should the developer path come before the other one, or is the intent to need to blank out the normal path to get the dev one?

Yeah back in the day it was called this.sessionSettings.developer.powerShellExePath that was changed to this.sessionSettings.powerShellExePath. Now Keith is changing that. Since VSCode doesn't notify when a setting has become deprecated (AFAIK) we support them as fallback until we want to remove support.

Keith, I totally agree v2 should remove this.

Well, I "think" the only change I made was to use the value of powerShellExePath if and only if it has already been set by the code above. If not, then we look in the same settings with the same fallback order as before. And the code above this will only ever set powerShellExePath on the initial startup of the extension and only if the setting powerShellDefaultVersion has been set.

TylerLeonhardt · 2018-04-16T22:45:33Z

src/session.ts

@@ -718,12 +736,25 @@ export class SessionManager implements Middleware {
    private showSessionMenu() {
        let menuItems: SessionMenuItem[] = [];

+        const currentExePath = (this.powerShellExePath || "").toLowerCase();


This is SUPER nitpicky (I'm sorry) but is there a reason this line comes before the getAvailablePowerShellExes call instead of closer to where it's first used in this function (aka between 743-744 after the if vs where it is now)?

No worries. I could move it below availablePowerShellExes but availablePowerShellExes is referenced before currentExePath. And both are used outside the if/else if statement

ahhh I see it below now. No need to change it.

TylerLeonhardt · 2018-04-16T22:51:34Z

test/platform.test.ts

@@ -5,6 +5,7 @@
 import * as assert from "assert";
 import * as vscode from "vscode";
 import * as platform from "../src/platform";
+import { SessionManager } from "../src/session";


Do you have to import this in order to pass undefined into the function call?

No I don't think so. That was probably a left-over line change from a previous approach. I can remove it.

rjmholt · 2018-04-16T23:09:18Z

src/platform.ts

                    return {
-                        versionName: `PowerShell Core ${path.parse(item).base}`,
+                        versionName: `PowerShell Core ${path.parse(item).base} ${arch}`,


exePath: path.join(item, "pwsh.exe"), below this avoids a variable and still gets readability here, provided I haven't missed anything

In fact it could be:

.map((item) => { versionName: `PowerShell Core ${path.parse(item),base} ${arch}`, exePath: path.join(item, "pwsh.exe") });

Yes, that is cleaner! Thanks. In removing that fs.existsSync() check, I should have seen the reduced form.

It was a little trickier than that but not by much.

Ah sorry, I was suspicious about my knowledge of TypeScript's lambda expression rules.

Minor tweak:

.map((item) => ({ versionName: `PowerShell Core ${path.parse(item),base} ${arch}`, exePath: path.join(item, "pwsh.exe") }));

An extra set of parens.

rjmholt · 2018-04-16T23:13:25Z

src/session.ts

+        powerShellExePath =
+            (powerShellExePath ||
+             this.sessionSettings.powerShellExePath ||
+             this.sessionSettings.developer.powerShellExePath ||


I'm assuming it's a fallback? Should the developer path come before the other one, or is the intent to need to blank out the normal path to get the dev one?

SeeminglyScience

LGTM

TylerLeonhardt · 2018-04-18T18:41:35Z

Nice work @rkeithhill! @markekraus will be very happy 😄

rkeithhill · 2018-04-18T21:42:02Z

I owed him for the basic auth work he did on IWR which we really needed in order to automate against our Artifactory-based repos.

TylerLeonhardt · 2018-04-18T21:43:30Z

it's amazing to have these kinds of interactions with open source :)

Implementation of additional exe paths

783752e

Fix #1243

rkeithhill requested review from TylerLeonhardt and daviwil April 16, 2018 01:37

TylerLeonhardt requested review from rjmholt and SeeminglyScience April 16, 2018 22:03

TylerLeonhardt approved these changes Apr 16, 2018

View reviewed changes

rjmholt approved these changes Apr 16, 2018

View reviewed changes

Address PR feedback

25685e2

SeeminglyScience approved these changes Apr 17, 2018

View reviewed changes

TylerLeonhardt merged commit 02225ca into master Apr 18, 2018

TylerLeonhardt deleted the rkeithhill/additional-ps-exes branch April 18, 2018 18:40

rjmholt mentioned this pull request Jun 11, 2018

PowerShell preview executables no longer at /usr/[local/]bin/pwsh on *nix #1361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of additional exe paths #1270

Implementation of additional exe paths #1270

rkeithhill commented Apr 16, 2018 •

edited

Loading

TylerLeonhardt left a comment

TylerLeonhardt Apr 16, 2018

rkeithhill Apr 16, 2018

TylerLeonhardt Apr 16, 2018

rkeithhill Apr 16, 2018

rjmholt Apr 16, 2018

TylerLeonhardt Apr 16, 2018

rkeithhill Apr 16, 2018

TylerLeonhardt Apr 16, 2018

rkeithhill Apr 16, 2018

TylerLeonhardt Apr 16, 2018

TylerLeonhardt Apr 16, 2018

rkeithhill Apr 16, 2018

rjmholt Apr 16, 2018

rjmholt Apr 16, 2018 •

edited

Loading

rkeithhill Apr 16, 2018

rkeithhill Apr 17, 2018

rjmholt Apr 17, 2018

rkeithhill Apr 17, 2018

rjmholt Apr 16, 2018

SeeminglyScience left a comment

TylerLeonhardt commented Apr 18, 2018 •

edited

Loading

rkeithhill commented Apr 18, 2018

TylerLeonhardt commented Apr 18, 2018

Implementation of additional exe paths #1270

Implementation of additional exe paths #1270

Conversation

rkeithhill commented Apr 16, 2018 • edited Loading

TylerLeonhardt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rjmholt Apr 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SeeminglyScience left a comment

Choose a reason for hiding this comment

TylerLeonhardt commented Apr 18, 2018 • edited Loading

rkeithhill commented Apr 18, 2018

TylerLeonhardt commented Apr 18, 2018

rkeithhill commented Apr 16, 2018 •

edited

Loading

rjmholt Apr 16, 2018 •

edited

Loading

TylerLeonhardt commented Apr 18, 2018 •

edited

Loading