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
feat: Elvish Shell support #1066
Conversation
Thanks for tackling this @elijahr :) |
Update docs
@jthegedus I've reworked this considerably, updated documentation, improved test coverage, and added shell completions. This is ready for review. |
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 mostly looks good to me, though I am not an elvish user so cannot validate myself locally. Given it does not change any major code paths for other shells I think we should merge and let future elvish users improve upon this as they find issues.
@Stratus3D can you give this a once over?
/installs | ||
/downloads | ||
/plugins | ||
shims | ||
/shims |
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.
How were you testing that required these changes?
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.
I added /downloads
because that directory gets populated by a call to asdf install
with the --keep-download
flag.
asdf install nim 1.4.8 --keep-download
I added leading slashes to the other entries because https://stackoverflow.com/a/38433961:
the leading slash anchors the match to the root
My understanding is that this makes sure other, nested items with the same name won't be excluded.
@jthegedus @Stratus3D I'd love for this to get merged/released. Please let me know if there is anything I can do to help that happen. |
@elijahr I would like another maintainer to review this before merge. Unfortunately we're all rather busy all the time. Thanks for your patience. |
.github/workflows/tests.yml
Outdated
# Build elvish and add to path | ||
git clone --depth 1 --single-branch https://github.com/elves/elvish | ||
cd elvish | ||
make get |
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.
Is this building elvish from source? Or is this just downloading a pre-built binary?
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.
Building from source - didn't take too long locally, but looks like there is a linux binary available, so using that instead. Changed in 2538279
Going to merge this as it is a large change and the longer we leave it the more annoying it will become to merge. We can hotfix |
Thanks @jthegedus and @elijahr ! Looks great! |
Summary
Add support for Elvish Shell.