fix(vscode): pick the print environment command by shell - #6091
Open
tripleaceme wants to merge 1 commit into
Open
tripleaceme wants to merge 1 commit into
tripleaceme wants to merge 1 commit into
Conversation
The print environment command opened a terminal with the user's default shell and then branched only on the platform, sending `set` on Windows and `env | sort` everywhere else. That assumes every non-Windows shell is a POSIX shell and every Windows shell is cmd. Resolve `vscode.env.shell` to a shell family instead and send the command that family accepts: `set` for fish, `env | sort` for POSIX shells, `set` for cmd and `Get-ChildItem Env: | Sort-Object Name` for PowerShell. `set` in PowerShell is an alias for `Set-Variable`, which prompts for a variable name rather than printing the environment, so the previous command did not work in the default Windows shell. An unrecognised shell falls back on the platform command, which keeps the previous behaviour for any shell not listed. Signed-off-by: Adegbite Ayoade <tripleaceme@gmail.com>
Contributor
Author
|
@cmgoffena13 — closes #5608. This is the option 1 I asked about on the issue (detect the shell); the description explains why I would argue for option 2 now that it is built, and the PR is shaped so switching is a deletion of one file and one import. The part worth your eye is not fish: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #5608.
sqlmesh: Print Environmentopens a terminal and sends a command to list the environment. It chose that command by platform, treating "not Windows" as "POSIX shell withenv":The command is now chosen from the shell that is actually in use. The decision lives in a pure function so it can be tested without a VS Code mock, and
printEnvironment.tsis one call into it.setsetGet-ChildItem Env: | Sort-Object NameUnrecognized shells fall back on the platform default, so nothing that works today stops working.
Two things worth knowing, both covered on the issue
The reporter's premise is not quite right. fish has real pipes and
envandsortare ordinary binaries there, soenv | sortdoes print the environment correctly in fish today. This is an idiomatic-output improvement, not a crash fix —setadditionally lists fish's own shell variables in fish's own formatting, which is what a fish user expects.The genuine breakage is on Windows, and the issue does not mention it. In PowerShell,
setis an alias forSet-Variable, which with no arguments does not print the environment — it promptsSupply values for the following parameters: Name[0]:and waits. PowerShell is VS Code's default terminal on Windows, so this command has been unusable for most Windows users. Fixed here because it is the same decision, but it does widen the change beyond the issue's text; happy to split it out if you would rather.I should be clear about confidence on that: it follows from
Set-Variable's documented behaviour rather than from a run. I have neither Windows nor fish available, so neither end of this is verified in a live shell, and the PowerShell replacement command is unverified too.Why
vscode.env.shellIt is stable API since 1.38 and this extension targets
^1.96.0. Its documentation says it is the detected default shell overridden byterminal.integrated.defaultProfile, so reading that setting directly would be strictly worse — it holds a profile name, not a path, and does not resolve the auto-detect case.process.env.SHELLis the extension host's login shell, which is not necessarily what VS Code launches, and is absent on Windows. Decisively, this command callscreateTerminalwith noshellPath, so the terminal it opens is exactly the onevscode.env.shelldescribes.Test Plan
13 tests in
src/utilities/shellCommand.test.ts, covering fish (including a Homebrew path), the POSIX family, cmd and PowerShell with and without.exe, a POSIX shell on Windows (Git Bash — POSIX wins over platform), a bare name with no path, mixed case, an unknown shell, and no shell reported at all (vscode.env.shellis''where none exists).Rather than reverting once, I checked the tests against three separate mutants, because reverting to the original behaviour leaves some tests green for the right reason:
isWindows ? 'set' : 'env | sort'setcmdmapped to a wrong valueUnion across the three: every one of the 13 tests fails under at least one mutant, so none is passing vacuously.
Nothing exercises
printEnvironment.tsitself, since there is no VS Code mock in this repo and I did not add one; the tests pin the decision function only.Checklist
make styleand fixed any issuesmake fast-test)git commit -s) per the DCO