Use `powerShellDefaultVersion` everywhere and stop using `powerShellExePath` #2304

TylerLeonhardt · 2019-11-14T00:10:46Z

PR Summary

fixes #2099

At long last, this PR removes the use of the powerShellExePath setting. @rkeithhill added an awesome feature a while back which allowed folks to be more granular with what version of PowerShell they want to use via the session menu.

This PR does the following:

Any operation that previously changed powerShellExePath now changes powerShellDefaultVersion
If the user has powerShellExePath and powerShellDefaultVersion set, a notification appears to remove the powerShellExePath from the settings.
If the user has powerShellExePath but not powerShellDefaultVersion, a notification appears to remove powerShellExePath and shows the session menu so the user can choose which version they want to use.

This does mess up users who have set powerShellExePath to "non well-known" locations... but I think those will be a small case... also, to help those folks and other folks who want to add additional PS exes, I added an item to the session menu that tells people they can modify a setting to add additional PS exes.

For this, it just jumps to the settings.json but in the future maybe we could have a little wizard that helps with adding them.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets.
Please mark anything not applicable to this PR NA.

PR has a meaningful title
Summarized changes
PR has tests
This PR is ready to merge and is not work in progress
- If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

JustinGrote · 2019-11-14T00:28:50Z

@TylerLeonhardt does this cover user settings, workspace settings, and folder settings and the precedence order therein (folder > workspace > user)?

TylerLeonhardt · 2019-11-14T00:49:38Z

This should only change the user setting of powershell.powerShellDefaultVersion and vscode should automatically give us the settings, upon loading the extension, in the correct precedence.

Has this not been the case?

…and remove references to developer.powerShellExePath

JustinGrote · 2019-11-14T01:58:41Z

Last I checked powershell.PowershellDefaultversion didn't work at the folder settings level (e.g. .vscode/settings.json), just seeing if this respects it as well.

Sorry, I'm really bad at reviewing Typescript :/

EDIT: But this has usually been because of PowershellExePath, maybe it'll work now. Adding a test maybe?

EDIT2: I forgot "workspace settings" encompasses both folder settings and workspace file settings. It should work fine. I'll download the .vsix from the azure pipelines build and try it out.

JustinGrote · 2019-11-14T02:58:35Z

Looks like most everything works. Workspace setting overrides user settings, and if you change user settings with workspace settings already set, it doesn't override it (as expected).

I'll code review for one specific behavior I think should be different, if I can find it...

JustinGrote · 2019-11-14T03:32:18Z

Found it.

vscode-powershell/src/settings.ts

Line 201 in 01c4a2b

await configuration.update(settingName, newValue, global);

So the settings change method always only writes to the user settings, which leads to the following undesirable scenario:

User sets workspace setting to Powershell Preview
User forgets they did this
User chooses the picker and chooses Windows Powershell (x64)
User wonders why their shell is still Powershell Preview after reload (because workspace setting still takes precedence), they keep clicking the toggle and it doesn't change, leading to frustration.

Try it, you keep trying to change the session and it keeps going back to whatever the workspace setting is, because only the user setting is getting updated but the workspace setting takes precedence.

The behavior for Session Menu should be (IMHO):

If only user settings is set, current behavior (update user settings)
If workspace/folder setting is set, it should update wherever the lowest precedence order setting is configured (folder > workspace file > user settings)

I'm not sure if this should be true for all powershell settings (adjust default change method behavior) or if it should just be true for this setting (add a new change method overload that addititionally allows specification of the settings scope, including an 'auto' that would detect the lowest level the setting has been set and update it at that level)

Caveat

The one rub here is that if a module author has set this in the workspace for his repository as an indicator to the user which powershell to use, it will get changed and potentially committed back to the repository by the user as they try out different shells.

The fix for this is probably out of scope (and just tell the user "dont do it" or block .vscode commit updates) but an idea could be something along the lines of:

Add additional workspace setting for project authors (e.g. powershell.powerShellRecommendedVersion yes/no) to indicate the powershell version they specified is what should be used for the project.
If powershell.powerShellRecommendedVersion is set and a workspace setting for powerShellDefaultVersion exists, if Session Menu is used, it should pop up a toast warning after selection mentioning the shell they have selected is not what the project author recommends, and then asking them if they want to switch anyways (yes/no).

In addition, potentially add setting to make shell changes nonpersistent, where it will switch to the shell but not update the workspace setting.

JustinGrote · 2019-11-14T21:27:52Z

Also: Potential regression - I've tried the CI build on two computers, and it prompted to remove PSExePath. After it prompted for reload it crashed with:

reference not set to an instance of an object. at Microsoft.PowerShell.EditorServices.Server.PsesLanguageServer.<>c.<<StartAsync>b__18_3>d.MoveNext() in D:\a\1\PowerShellEditorServices\src\PowerShellEditorServices\Server\PsesLanguageServer.cs:line 114 --- End of stack trace from previous location where exception was thrown --- at OmniSharp.Extensions.LanguageServer.Server.LanguageServer.MediatR.IRequestHandler<OmniSharp.Extensions.LanguageServer.Protocol.Models.InitializeParams,OmniSharp.Extensions.LanguageServer.Protocol.Models.InitializeResult>.Handle(InitializeParams request, CancellationToken token) at OmniSharp.Extensions.LanguageServer.Server.Pipelines.ResolveCommandPipeline`2.Handle(TRequest request, CancellationToken cancellationToken, RequestHandlerDelegate`1 next) at MediatR.Pipeline.RequestPostProcessorBehavior`2.Handle(TRequest request, CancellationToken cancellationToken, RequestHandlerDelegate`1 next)...

However doing a Developer: Reload window and it works fine. A minor item but may be worth investigating.

TylerLeonhardt · 2019-11-14T23:29:47Z

@JustinGrote thanks for pointing that out! That is an issue. I've addressed it in the last commit.

Here's what we do now - the session menu will always impact user settings. In the future we could have a nice wizard that does more... but for now, it will always impact user settings. That said, with that last commit we force the extension to start with the PowerShell exe that was chosen no matter what the workspace setting is set to.

TylerLeonhardt · 2019-11-14T23:35:50Z

Also: Potential regression

Looks like the crash came from PowerShellEditorServices... how consistent is that repro? I cant seem to repro at all.

JustinGrote · 2019-11-15T00:51:45Z

Not super consistent but the common denominator was neither powershellexepath or defaultpowershell were set.

…

On Thu, Nov 14, 2019, 3:35 PM Tyler James Leonhardt < ***@***.***> wrote: Also: Potential regression Looks like the crash came from PowerShellEditorServices... how consistent is that repro? I cant seem to repro at all. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2304?email_source=notifications&email_token=ADUNKUSODQYKBSGLR6TM5BDQTXOFZA5CNFSM4JNDO4UKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEEDW4LI#issuecomment-554135085>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADUNKUXKX24SJY7UFT2OFI3QTXOFZANCNFSM4JNDO4UA> .

TylerLeonhardt · 2019-11-16T06:40:19Z

Hmm interesting. I'll give it a try a few times but I don't think it's this change's fault. That's a PowerShell Editor Services issue.

This PR is ready for a real review now.

JustinGrote · 2019-11-19T01:10:36Z

@TylerLeonhardt #2311 might be a bug from this I ran into since I'm running the VSIX from this PR. So far I can consistently reproduce by starting the powershell editor in an "empty" vscode window (no folder opened) and starting a new powershell file.

TylerLeonhardt · 2019-11-19T01:35:05Z

nice find - got a PR out to fix it.

rjmholt · 2019-11-20T21:33:19Z

src/session.ts

@@ -691,53 +629,15 @@ export class SessionManager implements Middleware {
            SessionStatus.Failed);
    }

-    private changePowerShellExePath(exePath: string) {
+    private changePowerShellDefaultVersion(exePath: IPowerShellExeDetails) {


This looks like it would be a simple async method now

rjmholt · 2019-11-20T21:34:05Z

src/settings.ts

@@ -69,7 +69,7 @@ export interface IDebuggingSettings {

 export interface IDeveloperSettings {
    featureFlags?: string[];
-    powerShellExePath?: string;
+    // This setting is no longer used but is here to assist in cleaning up the users settings.


What setting does this comment apply to?

oops - removed that.

TylerLeonhardt added 2 commits November 13, 2019 15:34

Use powerShellDefaultVersion everywhere and stop using powerShellExePath

c7f97c5

add modify settings in session menu

600de8c

TylerLeonhardt requested review from rkeithhill and rjmholt November 14, 2019 00:10

JustinGrote mentioned this pull request Nov 14, 2019

[Feature] Workspace Setting for Integrated Console PS Version #2099

Closed

TylerLeonhardt added 2 commits November 13, 2019 16:51

re-add powerShellExePath to package.json so vscode api can update it …

47ccbba

…and remove references to developer.powerShellExePath

actually use setting...

01c4a2b

still switch even if workspace setting is set

92a496b

JustinGrote mentioned this pull request Nov 19, 2019

NullReferenceException from PSES if no folder is open when Preview Extension Loaded #2311

Closed

rjmholt reviewed Nov 20, 2019

View reviewed changes

rob's feedback

b598f7c

rjmholt approved these changes Nov 25, 2019

View reviewed changes

TylerLeonhardt merged commit b9eafbb into PowerShell:master Nov 26, 2019

TylerLeonhardt deleted the use-powershelldefaultversion-everywhere branch November 26, 2019 06:22

Use powerShellDefaultVersion everywhere and stop using powerShellExePath #2304

Use powerShellDefaultVersion everywhere and stop using powerShellExePath #2304

Uh oh!

Conversation

TylerLeonhardt commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

PR Checklist

Uh oh!

JustinGrote commented Nov 14, 2019

Uh oh!

TylerLeonhardt commented Nov 14, 2019

Uh oh!

JustinGrote commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JustinGrote commented Nov 14, 2019

Uh oh!

JustinGrote commented Nov 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Caveat

Uh oh!

JustinGrote commented Nov 14, 2019 • edited by rjmholt Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TylerLeonhardt commented Nov 14, 2019

Uh oh!

TylerLeonhardt commented Nov 14, 2019

Uh oh!

JustinGrote commented Nov 15, 2019 via email

Uh oh!

TylerLeonhardt commented Nov 16, 2019

Uh oh!

JustinGrote commented Nov 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TylerLeonhardt commented Nov 19, 2019

Uh oh!

rjmholt Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

rjmholt Nov 20, 2019

Choose a reason for hiding this comment

Uh oh!

TylerLeonhardt Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Use `powerShellDefaultVersion` everywhere and stop using `powerShellExePath` #2304

Use `powerShellDefaultVersion` everywhere and stop using `powerShellExePath` #2304

TylerLeonhardt commented Nov 14, 2019 •

edited

Loading

JustinGrote commented Nov 14, 2019 •

edited

Loading

JustinGrote commented Nov 14, 2019 •

edited

Loading

JustinGrote commented Nov 14, 2019 •

edited by rjmholt

Loading

JustinGrote commented Nov 19, 2019 •

edited

Loading