-
Notifications
You must be signed in to change notification settings - Fork 5
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
Add support for locating a bundled version of air
#166
Conversation
import * as vscode from "vscode"; | ||
import * as fs from "fs-extra"; | ||
|
||
export async function getRootWorkspaceFolder(): Promise<vscode.WorkspaceFolder> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also comes from https://github.com/microsoft/vscode-python-tools-extension-template/blob/8f474ec4ac4e7205ffed9f7f243473bb00bf29c0/src/common/utilities.ts#L38
It seems reasonable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some definition of reasonable ;)
I mean I would expect all this complexity to be hidden behind a VS Code API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. My main comment is that I think we should favour the bundled executable.
-
The formatter will eventually become very stable, so the chance of inconsistencies between the CLI and the LSP will be slim.
-
The bulk of Air features in the future will be in the LSP. And there will likely be several integrations that require the extension to match the executable. It would be very easy for a user to unknowingly get stuck with a CLI executable and not know they need to manually update it regularly.
editors/code/src/lsp.ts
Outdated
@@ -89,7 +99,7 @@ export class Lsp { | |||
|
|||
const config = vscode.workspace.getConfiguration( | |||
undefined, | |||
{ uri, languageId }, | |||
{ uri, languageId } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oops, commas are disappearing :P
import * as vscode from "vscode"; | ||
import * as fs from "fs-extra"; | ||
|
||
export async function getRootWorkspaceFolder(): Promise<vscode.WorkspaceFolder> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some definition of reasonable ;)
I mean I would expect all this complexity to be hidden behind a VS Code API.
7b84918
to
5659059
Compare
"bundled"
by defaultCloses #56
This PR goes with #164. Together it gives us the ability to release the Air extension to OpenVSX and the Code Marketplace with the air binary bundled in. It works for macOS (x86 and arm) and Windows (x86) right now.
A manual workflow run against this PR resulted in https://github.com/posit-dev/air/actions/runs/12796811126 with the artifacts you see at the bottom there. The Windows one worked out of the box on my Windows machine without having air installed! The Mac one works for me locally, I set
air.executableLocation: "bundled"
to easily test it.I'm very open to ideas to improve on this one. No strong feelings on the naming of the new
air.executableLocation
setting, but I do think we should probably default to"environment"
so by default we look in thePATH
and fallback to the bundled version if we can't find anything there. That should provide a more consistent experience when going from the command line to your IDE.It's quite easy to bundle a binary alongside your VS Code extension. You just place the binary in the extension folder and it gets slurped up. In the GitHub Action merged in #164 I placed it in
bundled/bin/<air/air.exe>
. This PR adds support for finding that bundled binary.I know we discussed not bundling for the first release but it didn't seem too hard and feels like it would just be a much nicer experience overall.