Skip to content
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

Merged
merged 34 commits into from Nov 18, 2021
Merged

feat: Elvish Shell support #1066

merged 34 commits into from Nov 18, 2021

Conversation

elijahr
Copy link
Contributor

@elijahr elijahr commented Oct 8, 2021

Summary

Add support for Elvish Shell.

@elijahr elijahr requested a review from a team as a code owner October 8, 2021 03:52
@elijahr elijahr changed the title Elvish Shell support feat: Elvish Shell support Oct 8, 2021
lib/asdf.elv Outdated Show resolved Hide resolved
@jthegedus
Copy link
Contributor

Thanks for tackling this @elijahr :)

@elijahr elijahr marked this pull request as draft October 9, 2021 03:56
@elijahr elijahr marked this pull request as ready for review October 9, 2021 22:30
@elijahr
Copy link
Contributor Author

elijahr commented Oct 9, 2021

@jthegedus I've reworked this considerably, updated documentation, improved test coverage, and added shell completions. This is ready for review.

Copy link
Contributor

@jthegedus jthegedus left a 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?

Comment on lines +1 to +4
/installs
/downloads
/plugins
shims
/shims
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@elijahr
Copy link
Contributor Author

elijahr commented Oct 28, 2021

@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.

@jthegedus
Copy link
Contributor

@elijahr I would like another maintainer to review this before merge. Unfortunately we're all rather busy all the time. Thanks for your patience.

# Build elvish and add to path
git clone --depth 1 --single-branch https://github.com/elves/elvish
cd elvish
make get
Copy link
Member

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?

Copy link
Contributor Author

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

asdf.fish Outdated Show resolved Hide resolved
@jthegedus
Copy link
Contributor

jthegedus commented Nov 18, 2021

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 head if any major issues. Thanks @elijahr

@jthegedus jthegedus merged commit cc7778a into asdf-vm:master Nov 18, 2021
@Stratus3D
Copy link
Member

Thanks @jthegedus and @elijahr ! Looks great!

@elijahr elijahr deleted the elvish branch November 18, 2021 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants