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

Fix bundler env #2595

Merged
merged 1 commit into from
Sep 23, 2024
Merged

Fix bundler env #2595

merged 1 commit into from
Sep 23, 2024

Conversation

epoberezhny
Copy link
Contributor

Motivation

Bundler has array configs like BUNDLE_WITHOUT and settings[e] returns array that later converted to string incorrectly (e.g. '[:development, :test]', but must be 'development:test'). Because of that bundler tries to install all gems in Gemfile, when ruby-lsp is not included in your application's Gemfile

Implementation

Automated Tests

Couldn't find any related tests, so tests are missing.

Manual Tests

You need to have a group of gems in your Gemfile. Then you should run bundle config set without 'YOUR_GROUP'. And you should not have gems in this group installed after the LSP server starts

@epoberezhny epoberezhny requested a review from a team as a code owner September 23, 2024 12:20
@epoberezhny
Copy link
Contributor Author

I've signed the CLA!

Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! We definitely missed this.

We should add a test for this to prevent regression. This test isn't exactly the same, but it also mutates project specific Bundler settings.

Can we please add a test case that configures any Bundler setting as an array so that we can confirm that the environment is mutated properly?

lib/ruby_lsp/setup_bundler.rb Outdated Show resolved Hide resolved
@vinistock vinistock added server This pull request should be included in the server gem's release notes bugfix This PR will fix an existing bug labels Sep 23, 2024
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Looks great! Let's just ensure we invoke tr too and we can ship

lib/ruby_lsp/setup_bundler.rb Outdated Show resolved Hide resolved
test/setup_bundler_test.rb Outdated Show resolved Hide resolved
@epoberezhny epoberezhny force-pushed the fix_bundler_env branch 2 times, most recently from 219eb88 to 82e4077 Compare September 23, 2024 17:14
Copy link
Member

@vinistock vinistock left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution!

@vinistock vinistock merged commit 81f08de into Shopify:main Sep 23, 2024
20 checks passed
@epoberezhny
Copy link
Contributor Author

Thank you for the contribution!

Thank you for creating Ruby LSP and quick responses!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants