Add support for Nushell and Murex#507
Conversation
jobukkit
requested review from
cmcgee1024,
etcwilde,
justice-adams-apple and
shahmishal
as code owners
| To begin using installed swiftly from your current shell, first run the following command: | ||
| \(sourceLine) | ||
|
|
||
| $ \(sourceLine.components(separatedBy: .newlines).last!) |
There was a problem hiding this comment.
praise: Thank you for cleaning up this long-standing problem where the comment is sent out along with the actual line to source swiftly.
| To begin using installed swiftly from your current shell, first run the following command: | ||
| \(sourceLine) | ||
|
|
||
| $ \(sourceLine.components(separatedBy: .newlines).last!) |
There was a problem hiding this comment.
issue (blocking): Can you remove the prefix dollar sign? It adds that extra bit of friction to copy and pasting the command. Without it the user can just select the whole line and paste. With it the user needs to carefully chop it out of the selection before copying.
There was a problem hiding this comment.
I didn't like the dollar signs either, but I saw them in some places so I assumed they were supposed to be everywhere. I have removed them now.
| your system: | ||
|
|
||
| \(command) | ||
| $ \(command) |
There was a problem hiding this comment.
issue: Please remove the dollar sign so that the user can just select the whole line, which is much easier than have to carefully select after the dollar sign to the end of the line. A ton of documentation is like this too, which is unfortunate.
| NOTE: Swiftly has updated some elements in your PATH and your shell may not yet be | ||
| aware of the changes. You can update your shell's environment by running | ||
|
|
||
| $ \(command) |
There was a problem hiding this comment.
issue: Same as above. Remove the dollar sign before the command to make it easier for users to copy, paste and run it.
| #expect(!migrations.filter { $0.matches(SwiftlyCore.version) }.isEmpty) | ||
| } | ||
|
|
||
| // TODO: add nu and murex binaries to test runner |
There was a problem hiding this comment.
issue: We'll need to update the GitHub workflows to ensure that we have integration testing with these new shells. Unit testing the config file generation isn't very effective on its own based on previous experiences with swiftly. If these shells can be apt-get installed into the containers and can be invoked with a login shell that might be enough to gain an extra bit of confidence that these don't accidentally break.
There was a problem hiding this comment.
See .github/workflows/pull_request.yml in the releasetest workflow.
| } | ||
| } | ||
|
|
||
| // TODO: add nu and murex binaries to test runner |
There was a problem hiding this comment.
issue: Swiftly uninstall is a relatively new feature, and if possible I would like the experience to be good for users who don't want to use swiftly anymore. At least their final impression that it returned their system back to the way it was before without errors, leaving them to manually edit their profiles.
There was a problem hiding this comment.
Sorry, I am not sure what issue you are referring to.
|
Thank you for your contribution. Overall, this is good. I've posted some questions, and issues. |
This comment was marked as resolved.
This comment was marked as resolved.
2 participants
This patch adds support for automatically configuring Nushell and Murex shell profiles in addition to the current support for Bash/Zsh/Fish (and makes the README list all these under "Platform support").
Also, during
initthe user is now told which shell profile file is getting added to, and duringself-uninstallFish users are no longer left with an emptyswiftly.fishfile (it's deleted if the user didn't add anything to it).I have manually tested initializing, using, and uninstalling this version of swiftly under both new shells on both MacOS and Linux. I haven't ran the automated tests. Nushell and Murex binaries would have to be added to the test runner.
Obsoletes #284.