Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Implementation of additional exe paths #1270

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

Merged
TylerLeonhardt merged 2 commits into master from rkeithhill/additional-ps-exes
Apr 18, 2018

Conversation

Copy link
Contributor

@rkeithhill rkeithhill commented Apr 16, 2018 •
edited
Loading

Fix #1243

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:

image

And after:

image

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"

rjmholt, TylerLeonhardt, and markekraus reacted with hooray emoji
Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM Keith! Just had a few questions 😄

// 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);
Copy link
Member

@TylerLeonhardt TylerLeonhardt Apr 16, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rkeithhill rkeithhill Apr 16, 2018

Choose a reason for hiding this comment

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

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

TylerLeonhardt reacted with thumbs up emoji
powerShellExePath =
(powerShellExePath ||
this.sessionSettings.powerShellExePath ||
this.sessionSettings.developer.powerShellExePath ||
Copy link
Member

@TylerLeonhardt TylerLeonhardt Apr 16, 2018

Choose a reason for hiding this comment

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

this is for backcompat?

Copy link
Contributor Author

@rkeithhill rkeithhill Apr 16, 2018

Choose a reason for hiding this comment

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

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

TylerLeonhardt reacted with thumbs up emoji
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

@TylerLeonhardt TylerLeonhardt Apr 16, 2018

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@rkeithhill rkeithhill Apr 16, 2018

Choose a reason for hiding this comment

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

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 and rjmholt reacted with thumbs up emoji
private showSessionMenu() {
let menuItems: SessionMenuItem[] = [];

const currentExePath = (this.powerShellExePath || "").toLowerCase();
Copy link
Member

@TylerLeonhardt TylerLeonhardt Apr 16, 2018

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor Author

@rkeithhill rkeithhill Apr 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Member

@TylerLeonhardt TylerLeonhardt Apr 16, 2018

Choose a reason for hiding this comment

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

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

import * as assert from "assert";
import * as vscode from "vscode";
import * as platform from "../src/platform";
import { SessionManager } from "../src/session";
Copy link
Member

@TylerLeonhardt TylerLeonhardt Apr 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@rkeithhill rkeithhill Apr 16, 2018

Choose a reason for hiding this comment

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

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

TylerLeonhardt reacted with thumbs up emoji
src/platform.ts Outdated
const exePath = path.join(item, "pwsh.exe");
return {
versionName: `PowerShell Core ${path.parse(item).base}`,
versionName: `PowerShell Core ${path.parse(item).base}${arch}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@rjmholt rjmholt Apr 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In fact it could be:

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

TylerLeonhardt reacted with thumbs up emoji
Copy link
Contributor Author

@rkeithhill rkeithhill Apr 16, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@rkeithhill rkeithhill Apr 17, 2018

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@rkeithhill rkeithhill Apr 17, 2018

Choose a reason for hiding this comment

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

Minor tweak:

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

An extra set of parens.

powerShellExePath =
(powerShellExePath ||
this.sessionSettings.powerShellExePath ||
this.sessionSettings.developer.powerShellExePath ||
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

@SeeminglyScience SeeminglyScience left a comment

Choose a reason for hiding this comment

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

LGTM

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

TylerLeonhardt commented Apr 18, 2018 •
edited
Loading

Nice work @rkeithhill! @markekraus will be very happy 😄
rkeithhill reacted with laugh emoji

Copy link
Contributor Author

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 reacted with thumbs up emoji markekraus and rjmholt reacted with laugh emoji

Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SeeminglyScience SeeminglyScience SeeminglyScience approved these changes

@daviwil daviwil Awaiting requested review from daviwil

+2 more reviewers

@TylerLeonhardt TylerLeonhardt TylerLeonhardt approved these changes

@rjmholt rjmholt rjmholt approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /