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

Automatically install ruby-lsp-rails as part of custom bundle #1381

Merged
merged 1 commit into from
Feb 16, 2024

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Feb 14, 2024

Motivation

With ruby-lsp-rails v0.3.0, we switched away from the mounted Rack application approach. This means ruby-lsp-rails can now be used without having to add it to the app's Gemfile.

Also closes #1267

Implementation

Previously we would install debug and ruby-lsp into the custom bundle. We now also install ruby-lsp-rails if the app uses Rails.

Automated Tests

Updated

Manual Tests

Verified.

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.

Let's write a PR description, but the changes are good

@andyw8
Copy link
Contributor Author

andyw8 commented Feb 14, 2024

@vinistock I think we need a few more changes, e.g.

return false if @dependencies["ruby-lsp"] && @dependencies["debug"]

@vinistock
Copy link
Member

Ah, yeah. True. We need to add ruby-lsp-rails to that check.

@andyw8 andyw8 force-pushed the andyw8/auto-install-ruby-lsp-rails branch 3 times, most recently from 282437b to ae97b57 Compare February 14, 2024 20:08
)

# If the user decided to add the `ruby-lsp` and `debug` to their Gemfile after having already run the Ruby LSP,
# If the user decided to add `ruby-lsp`, `ruby-lsp-rails` and `debug` to their Gemfile after having already run the Ruby LSP,
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'll need to reformat some of these comments to due the the line length, but I'll wait until the review is done.

@andyw8
Copy link
Contributor Author

andyw8 commented Feb 14, 2024

If there was a chance we might need to add a fourth gem here, then I would consider refactoring this, since we need to repeat the list of 3 in many places currently.

@andyw8 andyw8 force-pushed the andyw8/auto-install-ruby-lsp-rails branch from ae97b57 to 3b5cf6e Compare February 14, 2024 20:12
@vinistock
Copy link
Member

Looks good to me

@andyw8
Copy link
Contributor Author

andyw8 commented Feb 14, 2024

(once this is merged, we can update the ruby-lsp-rails README to indicate that it doesn't need to be added to the Gemfile).

@andyw8 andyw8 marked this pull request as ready for review February 15, 2024 16:57
@andyw8 andyw8 requested a review from a team as a code owner February 15, 2024 16:57
@andyw8 andyw8 requested review from Morriar and st0012 February 15, 2024 16:57
@andyw8 andyw8 changed the title WIP: Automatically install ruby-lsp-rails Automatically install ruby-lsp-rails as part of custom bundle Feb 15, 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.

Let's add one test for the update case (which would've caught the logic issue).

lib/ruby_lsp/setup_bundler.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/setup_bundler.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/setup_bundler.rb Outdated Show resolved Hide resolved
lib/ruby_lsp/setup_bundler.rb Outdated Show resolved Hide resolved
test/setup_bundler_test.rb Outdated Show resolved Hide resolved
test/setup_bundler_test.rb Outdated Show resolved Hide resolved
@andyw8 andyw8 force-pushed the andyw8/auto-install-ruby-lsp-rails branch from 2c2c236 to 67ee399 Compare February 16, 2024 17:59
@andyw8 andyw8 force-pushed the andyw8/auto-install-ruby-lsp-rails branch from 67ee399 to dc41d3a Compare February 16, 2024 18:31
@andyw8 andyw8 force-pushed the andyw8/auto-install-ruby-lsp-rails branch from dc41d3a to 662dfba Compare February 16, 2024 19:48
@andyw8 andyw8 enabled auto-merge (squash) February 16, 2024 19:50
@andyw8 andyw8 merged commit e43ac52 into main Feb 16, 2024
30 checks passed
@andyw8 andyw8 deleted the andyw8/auto-install-ruby-lsp-rails branch February 16, 2024 19:57
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.

2 participants