Skip to content

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

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

vinistock
Copy link
Member

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

  • We keep the version manager setting, so that people can specify what they are using
  • We make each version manager a separate class, which is responsible for processing the configuration files and finding the Ruby installation directory (which are both different in most managers)
  • The parent version manager enforces what needs to be implemented by each version manager and puts all the pieces together to activate the Ruby environment

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

  1. Launch the extension in a project using chruby
  2. Verify that the LSP launches correctly

@vinistock vinistock added enhancement New feature or request vscode This pull request should be included in the VS Code extension's release notes labels Feb 29, 2024
@vinistock vinistock self-assigned this Feb 29, 2024
@vinistock vinistock requested a review from a team as a code owner February 29, 2024 22:54
@vinistock vinistock requested review from st0012 and egiurleo February 29, 2024 22:54
@vinistock vinistock force-pushed the vs/migrate_chruby_activation branch from fb8f892 to 66cca1c Compare February 29, 2024 22:57
@vinistock vinistock requested a review from andyw8 February 29, 2024 23:08
Copy link
Member

@st0012 st0012 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎩 👌

@vinistock vinistock force-pushed the vs/migrate_chruby_activation branch 2 times, most recently from fbb6d0a to 6d2bcb9 Compare March 1, 2024 18:34
@vinistock vinistock force-pushed the vs/migrate_chruby_activation branch from 6d2bcb9 to 9059144 Compare March 10, 2024 19:50
@vinistock vinistock changed the base branch from main to vs/migrate_shadowenv_activation March 10, 2024 19:50
@vinistock vinistock force-pushed the vs/migrate_shadowenv_activation branch from 64e1893 to ea2bc91 Compare March 11, 2024 17:35
Base automatically changed from vs/migrate_shadowenv_activation to main March 12, 2024 18:46
@vinistock vinistock force-pushed the vs/migrate_chruby_activation branch from 9059144 to d1a19c5 Compare March 12, 2024 18:48
await this.runActivationScript(rubyUri);

this.outputChannel.info(
`Activated Ruby environment: defaultGems=${defaultGems} gemHome=${gemHome} yjit=${yjit}`,
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

@andyw8 andyw8 left a 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.

@vinistock vinistock force-pushed the vs/migrate_chruby_activation branch from d1a19c5 to fffaa4e Compare March 12, 2024 19:48
@vinistock vinistock merged commit f18a7d4 into main Mar 12, 2024
@vinistock vinistock deleted the vs/migrate_chruby_activation branch March 12, 2024 20:29
@tvdeyen
Copy link

tvdeyen commented Mar 13, 2024

This broke chruby detection for me. I HAVE a .ruby-version in my repo (a gem, not a Rails app) and I have a global ~/.ruby-version file in my home folder. It works everywhere, but not with this version of the ruby-lsp extension in latest VSCode. It was working perfectly until this change.

@tvdeyen
Copy link

tvdeyen commented Mar 13, 2024

Next person googling here I pinned the extension to 0.5.12 and the issue is gone.

https://stackoverflow.com/questions/42626065/vs-code-how-to-rollback-extension-install-specific-extension-version

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request vscode This pull request should be included in the VS Code extension's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants