-
Notifications
You must be signed in to change notification settings - Fork 196
Migrate chruby activation to the new version manager implementation #1438
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
Conversation
fb8f892
to
66cca1c
Compare
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.
🎩 👌
fbb6d0a
to
6d2bcb9
Compare
6d2bcb9
to
9059144
Compare
64e1893
to
ea2bc91
Compare
9059144
to
d1a19c5
Compare
await this.runActivationScript(rubyUri); | ||
|
||
this.outputChannel.info( | ||
`Activated Ruby environment: defaultGems=${defaultGems} gemHome=${gemHome} yjit=${yjit}`, |
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.
Could this be in the parent class? Since I expect we'll have something similar for other managers.
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.
Let's wait a bit until we have more managers using this format. The issue is that some of them will be implemented directly in TypeScript and others will just invoke an executable. For the executable ones, printing the entire environment is too large and not very useful.
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.
Tophatted against factory_bot
That repo doesn't have a .ruby-version
file, so it initially detected my ~/.ruby-version
, as it should.
I then added a .ruby-version
with a different version and restarted the LSP, and it picked that up.
d1a19c5
to
fffaa4e
Compare
This broke chruby detection for me. I HAVE a |
Next person googling here I pinned the extension to 0.5.12 and the issue is gone. |
Motivation
The changes in Shopify/vscode-ruby-lsp#923 were useful for testing purposes, but I think we cannot have a blanket implementation that will work for every version manager. Clearly, they each have their own specificities that need to be accounted for with care (e.g.: Mise supports multiple level of hierarchy in configuration and RVM supports the .ruby-gemset file).
To move forward with more confidence, I'm proposing a similar approach, with a redesign, so that we can ship this as part of our stable releases.
Implementation
The idea of the implementation is that
This way, we can also move slowly and ship one version manager at a time, gathering feedback as we go.
This maintains the similar advantages of the other PR, such as fully decoupling from shells and allowing us to have extensive testing for each manager. Additionally, this PR implements most of the shared behaviour, so the next managers should be easier to implement.
Automated Tests
Added tests for chruby.
Manual Tests