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.

Sign up for GitHub

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

Jump to bottom

Improve Rails app detection #1467

Closed
wants to merge 1 commit into from
Closed

Improve Rails app detection #1467

wants to merge 1 commit into from

Conversation

andyw8
Copy link
Contributor

@andyw8 andyw8 commented Mar 6, 2024 •
edited
Loading

Motivation

Closes #1466

Implementation

Check for the presence of bin/rails as suggested by Rafael.

Automated Tests

Included

Manual Tests

It's slightly awkward as we need to ensure the setup process doesn't end up installing the published version:

  • Check out this branch.
  • Temporarily change the version to something nonsense value, e.g. 0.14.999 so you can easily identify it.
  • Run gem build
  • Run gem install <filename>
  • Check out the rails/rails repo.
  • Delete the current .ruby-lsp directory if it exists
  • Reload the window
  • Ensure the Ruby LSP output window doesn't show a failure for bin/rails
  • Ensure .ruby-lsp/Gemfile doesn't contain ruby-lsp-rails
  • Remember to uninstall the temporary local gem afterwards, or it may result in confusing failures.

@andyw8 andyw8 added the server This pull request should be included in the server gem's release notes label Mar 6, 2024
andyw8
andyw8 commented Mar 6, 2024
View reviewed changes
test/setup_bundler_test.rb
@@ -12,11 +12,10 @@ def test_does_nothing_if_both_ruby_lsp_and_debug_are_in_the_bundle
refute_path_exists(".ruby-lsp")
end

def test_does_nothing_if_both_ruby_lsp_and_debug_are_in_the_bundle2
def test_does_nothing_if_both_ruby_lsp_and_ruby_lsp_rails_are_in_the_bundle
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgot to update method name in #1381

andyw8
andyw8 commented Mar 6, 2024
View reviewed changes
test/setup_bundler_test.rb
@@ -479,6 +481,8 @@ def test_ruby_lsp_rails_is_automatically_included_in_rails_apps
source "https://rubygems.org"
gem "rails"
GEMFILE
FileUtils.mkdir(File.join(dir, "bin"))
Copy link
Contributor Author

@andyw8 andyw8 Mar 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(within the mktmpdir block so no need to manually delete)

@andyw8 andyw8 added the bugfix This PR will fix an existing bug label Mar 6, 2024
@andyw8 andyw8 mentioned this pull request Mar 8, 2024
Merged
@andyw8
Copy link
Contributor Author

andyw8 commented Mar 8, 2024

There is some corresponding discussion happening in Shopify/ruby-lsp-rails#284 about what is the right approach for this.

@andyw8 andyw8 changed the title WIP: Improve Rails app detection Improve Rails app detection Mar 8, 2024
@andyw8 andyw8 force-pushed the andyw8/improve-rails-app-detection branch from 4013be3 to 43aa280 Compare March 8, 2024 16:51
@andyw8 andyw8 marked this pull request as ready for review March 8, 2024 16:51
@andyw8 andyw8 requested a review from a team as a code owner March 8, 2024 16:51
@andyw8 andyw8 requested review from egiurleo, KaanOzkan, st0012 and vinistock and removed request for KaanOzkan March 8, 2024 16:51
@andyw8 andyw8 force-pushed the andyw8/improve-rails-app-detection branch from 43aa280 to a8b5e5c Compare March 8, 2024 16:53
@vinistock
Copy link
Member

My two cents is that, as long as the ruby-lsp-rails is not failing, we can delay this decision a little bit because we may find value in having the addon present for Ruby apps that have rails in the Gemfile, but aren't Rails applications.

@andyw8
Copy link
Contributor Author

andyw8 commented Apr 4, 2024

Handled by Shopify/ruby-lsp-rails#284

@andyw8 andyw8 closed this Apr 4, 2024
@Earlopain Earlopain mentioned this pull request Apr 8, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@egiurleo egiurleo Awaiting requested review from egiurleo egiurleo was automatically assigned from Shopify/ruby-dev-exp

@st0012 st0012 Awaiting requested review from st0012

@vinistock vinistock Awaiting requested review from vinistock

Assignees
No one assigned
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Improve how we detect Rails apps
2 participants
@andyw8 @vinistock