Add new additionalPowerShellLocations option#5484
Conversation
c7a0c8d to
12ddce1
Compare
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
12ddce1 to
41a2b1d
Compare
There was a problem hiding this comment.
Pull request overview
Adds a new cross-platform-friendly configuration option (powershell.additionalPowerShellLocations) to let users define additional PowerShell executables with per-platform (OS+arch) filtering and weighted ranking, integrating it into PowerShell discovery and the Session Menu flow.
Changes:
- Added
SupportedPlatform,IAdditionalPowerShellLocation, andgetSupportedPlatform()plus a new enumeration path for additional locations inPowerShellExeFinder. - Updated
SessionManagerto preferadditionalPowerShellLocationsover legacy settings and to adjust entryweightwhen switching via Session Menu. - Added unit tests for supported-platform mapping and additional-location filtering/sorting; added the new setting schema to
package.json.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/platform.ts |
Introduces the new platform/arch mapping + additional-location enumeration and integrates it into PowerShell discovery. |
src/session.ts |
Reads the new setting, uses it to derive the requested PowerShell, and updates weight on Session Menu selection. |
test/core/platform.test.ts |
Adds coverage for platform mapping and additional-location filtering/priority behavior. |
package.json |
Adds powershell.additionalPowerShellLocations configuration schema and documentation. |
| const configuredPowerShells = | ||
| this.additionalPowerShellLocations.length > 0 | ||
| ? this.enumerateAdditionalPowerShellLocations() | ||
| : this.enumerateAdditionalPowerShellInstallations(); | ||
| for await (const additionalPwsh of configuredPowerShells) { |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
additionalPowerShellLocations optionadditionalPowerShellLocations option
Signed-off-by: Chawye Hsu <su+git@chawyehsu.com>
additionalPowerShellLocations optionadditionalPowerShellLocations option
|
By any chance can I get some reviews? @JustinGrote @andyleejordan |
andyleejordan
left a comment
There was a problem hiding this comment.
Just stopping by with a decision question: would there be a good way to handle the "default" cases of just windows/macos/linux (without architecture)? My thought is, and this is really an open question: are most people going to want or need to specify the architecture, especially as x64 is kinda on its way out (I mean look at macOS, you'd be hardpressed to find anything but Apple Silicon now). I'm thinking perhaps handle those as defaults, with architecture specific versions as options too.
On the note of these settings getting complicated, do we have a simpler way to handle "weight" that doesn't require the user to specify? I would think the ordered list of options would be implicit weights. Perhaps what this should be is three settings: Windows paths, macOS paths, Linux paths. Infer the priority from the order of paths in the list. Have an optional field on the subproperty (the required field being the path) that is the architecture. Eh?
@JustinGrote could you weigh in on this? Really just spitballing how to make this as clean of a user facing settings interface as possible.
PR Summary
Implement and close #5233
This PR attempts to add a better (cross-platform friendly) way to configure additional PowerShell paths.
Details
This implementaion is a little different from what I've proposed last year in #5233, the primary difference is instead of a
default?: booleanfield, aweight?: numberis used to rank entries and select the best matching entry. Since we cannot restrict thedefault: trueper platform to only one in the VS Code extension configuration, using weighted sorting allows us to achieve a similar purpose. Furthermore, the more specificplatformfield involvingarchis used to specify the target host instead ofos.Backward Compatibility? Yes
While the old
powerShellAdditionalExePathsandpowerShellDefaultVersionstay unchanged and can still be used to define additional PowerShell paths, the newadditionalPowerShellLocationsis added to be mutually exclusive, which means whenadditionalPowerShellLocationsis defined(non-empty), old options are ignored. This keeps settings non-conflicting: users pick one path (old or new), not both. Intuitive, simple and easy to understand.Old options could be marked deprecated after a transition period.
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