Install Toolchain #1780
Install Toolchain #1780
Conversation
|
roulpriya
force-pushed
the
installing-swiftly-toolchain
branch
from
2bf9753 to
58ff42b
Compare
roulpriya
force-pushed
the
installing-swiftly-toolchain
branch
from
0b32ffd to
845d30a
Compare
roulpriya
force-pushed
the
installing-swiftly-toolchain
branch
from
845d30a to
3b13b09
Compare
roulpriya
requested review from
adam-fowler,
0xTim and
award999
as code owners
There was a problem hiding this comment.
Looking really good so far! Here are some things I noticed while testing this out locally:
- The toolchain selection dialog is getting really big with this change. I think it makes more sense to make the Swiftly install a separate VS Code command and add it as an action to the toolchain selection dialog. Toolchain selection is more about switching between already installed toolchains anyway.
- Toolchains should be ordered most recent first rather than earliest. E.g. 6.1.1 should be at the top.
| * Shows a quick pick dialog to install available Swiftly toolchains | ||
| */ | ||
| export async function installSwiftlyToolchain(ctx: WorkspaceContext): Promise<void> { | ||
| if (!Swiftly.isSupported()) { |
There was a problem hiding this comment.
todo: through this command's invocation: whenever we're showing an error or info message, we can log it so we can trace later
| // Sort toolchains with most recent versions first | ||
| const sortedToolchains = sortToolchainsByVersion(uninstalledToolchains); | ||
|
|
||
| const quickPickItems = sortedToolchains.map(toolchain => ({ |
There was a problem hiding this comment.
suggestion: could log available toolchains so we know what they were prompted with
| } | ||
|
|
||
| try { | ||
| await vscode.window.withProgress( |
There was a problem hiding this comment.
todo: some logging of what will be installed, the command, etc.
| ); | ||
|
|
||
| void showReloadExtensionNotification( | ||
| `Swift ${selected.toolchain.name} has been installed and activated. Visual Studio Code needs to be reloaded.` |
There was a problem hiding this comment.
| `Swift ${selected.toolchain.name} has been installed and activated. Visual Studio Code needs to be reloaded.` | |
| `Swift ${selected.toolchain.name} has been installed and selected as the active toolchain. Visual Studio Code needs to be reloaded.` |
| }); | ||
| } | ||
|
|
||
| function parseStableVersion( |
There was a problem hiding this comment.
todo: Version#fromString static function has this logic
There was a problem hiding this comment.
could extend to handle extractSnapshotDate if needed
| if (shouldExecute) { | ||
| await this.executePostInstallScript(postInstallFilePath, version, logger); | ||
| } else { | ||
| void vscode.window.showWarningMessage( |
There was a problem hiding this comment.
todo: should log too for diagnostics
| `This will require administrator privileges.\n\n${validation.summary}\n\n` + | ||
| `Do you want to proceed with running the post-install script?`; | ||
|
|
||
| const choice = await vscode.window.showWarningMessage( |
| ): Promise<void> { | ||
| logger?.info(`Executing post-install script for toolchain ${version}`); | ||
|
|
||
| const outputChannel = new SwiftOutputChannel( |
There was a problem hiding this comment.
question: was this a suggestion from someone? Not sure if we want to open a new output channel, but at very least wouldn't we want this to be in our log folder so it's archived?
src/ui/ToolchainSelection.ts
Outdated
| /** | ||
| * Sorts toolchains by version with most recent first | ||
| */ | ||
| function sortToolchainsByVersion<T extends { label: string }>(toolchains: T[]): T[] { |
There was a problem hiding this comment.
question: seems like close to same logic is in installSwiftlyToolchain.ts?
src/ui/ToolchainSelection.ts
Outdated
| return a.name.localeCompare(b.name); | ||
| } | ||
|
|
||
| function parseStableVersion( |
src/ui/ToolchainSelection.ts
Outdated
| return null; | ||
| } | ||
|
|
||
| function extractSnapshotDate(version: string): string | null { |
Feature Issue #1778
Description
Issue: 1778
Tasks