Add Swiftly installation support #1881
Conversation
roulpriya
force-pushed
the
swiftly-install-via-vscode
branch
from
7905f88 to
bbbe271
Compare
roulpriya
force-pushed
the
swiftly-install-via-vscode
branch
from
bbbe271 to
d6cb6ce
Compare
roulpriya
requested review from
0xTim,
adam-fowler and
award999
as code owners
roulpriya
force-pushed
the
swiftly-install-via-vscode
branch
from
d6cb6ce to
582b344
Compare
matthewbastien
left a comment
matthewbastien
left a comment
There was a problem hiding this comment.
Looking good so far, thank you! I have several comments from my first look at this.
You'll also have to rebase on top of main before adding to the changelog.
| "order": 4, | ||
| "scope": "machine-overridable" | ||
| }, | ||
| "swift.suppressSwiftlyInstallPrompt": { |
There was a problem hiding this comment.
suggestion: Just to keep this consistent with other setting names:
| "swift.suppressSwiftlyInstallPrompt": { | |
| "swift.disableSwiftlyInstallPrompt": { |
| /** | ||
| * Checks if any workspace folder contains a .swift-version file | ||
| */ | ||
| async function hasSwiftVersionFile(): Promise<boolean> { |
There was a problem hiding this comment.
suggestion: We should probably also check the FolderContext directories since a user can open a top level folder with multiple sub-folders that have a Package.swift. Maybe even just make this check happen at the FolderContext or WorkspaceContext level?
| ): Promise<SwiftToolchain | undefined> { | ||
| // Check if there's a .swift-version file in the workspace and Swiftly is not installed | ||
| if (await hasSwiftVersionFile()) { | ||
| const { Swiftly } = await import("./toolchain/swiftly"); |
There was a problem hiding this comment.
issue: Keep the imports at the top level. Makes it easier to see where the dependencies are.
| }); | ||
| }); | ||
|
|
||
| suite("installSwiftly()", () => { |
There was a problem hiding this comment.
issue: These tests appear to actually be attempting to decompress artifacts judging by some of the error messages in CI. You'll have to find some way to abstract away tar with dependency injection. It honestly might be easier to do an integration test for the install. However, performing a swiftly init is going to mess with environment variables which could pose problems.
A bunch of these tests are failing in CI which will need to be fixed up.
2 participants
Description
Install Swiftly from extension
Issue: Please provide reference link to the Github issue
Tasks