Conversation
thePianoKid
requested review from
0xTim,
adam-fowler and
award999
as code owners
rbenegal
left a comment
rbenegal
left a comment
There was a problem hiding this comment.
Thanks for this! Looking good, left some comments for your consideration.
There was a problem hiding this comment.
Thanks for this, I was thinking the first "Install Swift" step could also have an Install Swiftly and Swift Toolchain button instead of being told to manually install from swift.org. Possibly can detect whether the user has an existing toolchain (or Xcode installed) and if they don't they can click a button and have the extension set everything up?
assets/walkthrough/swiftly.md
Outdated
|
|
||
|  | ||
|
|
||
| A toolchain is the complete set of tools needed to compile and run Swift code. [Swiftly](https://www.swift.org/swiftly/documentation/swiftlydocs/) is a toolchain manager and installer that allows you to switch between different toolchains. If you're familiar with the JS ecosystem, Swiftly is roughly equivalent to [nvm](https://www.nvmnode.com). |
There was a problem hiding this comment.
I'm not sure if a comparison with other tools is required here (there's plenty of other similar tools like rustup etc. and hard to say what the user would be familiar with). Probably best to stick with just swiftly.
src/commands/installSwiftly.ts
Outdated
| } | ||
| } | ||
|
|
||
| export async function runInstallSwiftly(logger?: SwiftLogger): Promise<boolean> { |
There was a problem hiding this comment.
This seems to be a copy of promptForSwiftlyInstallation with slight changes? Wondering if it would be better to refactor to avoid duplication for future maintenance possibly?
thePianoKid
force-pushed
the
add-swiftly-to-walkthrough
branch
from
b090715 to
e8352a7
Compare
|
After an offline conversation with @rbenegal, we decided to add this step to the first step on the welcome page. I've refactored accordingly. |
src/extension.ts
Outdated
| }); | ||
|
|
||
| const installCommand = vscode.commands.registerCommand( | ||
| "sswg.swift.installSwift", |
There was a problem hiding this comment.
We should not be using sswg.swift anymore since that was the name of the old deprecated swift extension. Probably just swift.installSwift is good. Also this should be done in commands.ts and not here I believe.
| // Show the error dialog asynchronously without blocking activation. | ||
| // This prevents a UI deadlock where the walkthrough buttons can't work | ||
| // because activation is blocked waiting for the user to dismiss the dialog. | ||
| void showToolchainError().then(chosenRemediation => { |
There was a problem hiding this comment.
I found this was the only way to get the "Install Swift" button to work while there was no Swift toolchain installed (which is the most important time for that button to work!)
| const swiftlyCheckElapsed = Date.now() - swiftlyCheckStartTime; | ||
|
|
||
| const swiftLangCheckStartTime = Date.now(); | ||
| await checkForSwiftLangInstallation(); |
There was a problem hiding this comment.
So that we can render a checkmark when swift --version returns something.
3 participants
Description
Now that Swiftly support has been integrated into the extension, it makes sense to add it to the welcome walkthrough. This PR adds a new step to the welcome walkthrough that explains what a toolchain is, what Swiftly is, and includes a button users can press to install Swiftly on their system.
Issue: partially addresses #1561
Tasks