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

Remove RuboCop dependency and fallback to SyntaxTree formatting #184

Merged
merged 4 commits into from
Jul 7, 2022

Conversation

vinistock
Copy link
Member

Motivation

Closes #76
Building on top of #183

If RuboCop is not present in the current application, we can fallback to formatting using SyntaxTree. Additionally, this allows us to drop the dependency on RuboCop and simply try to require it when available.

Implementation

Both the diagnostics and formatting runners will now exit early without defining the classes if RuboCop is not present. If the runner classes don't exist, then we can exit early in diagnostics and fallback to SyntaxTree on formatting.

Automated Tests

Honestly, I'm not sure about the best way to test this. If you have ideas on how to fake that RuboCop is not present in the bundle, please let me know.

Manual Tests

It's a bit of work, but here are the steps if you're interested in testing it out.

With RuboCop

  1. On the Ruby LSP project itself (which uses RuboCop), start the server
  2. Edit some files
  3. Verify that RuboCop diagnostics are showing up
  4. Verify that formatting the file fixes the RuboCop violations

Without RuboCop

  1. Create a minimal project structure (Gemfile, dev.yml and an example.rb file)
  2. Configure dev.yml with the desired Ruby version and dev up
  3. In the Gemfile, add the Ruby LSP pointing to this branch (e.g.: gem "ruby-lsp", path: "../ruby-lsp")
  4. Start the plugin
  5. Edit example.rb
  6. Verify no diagnostics show up
  7. Verify that saving the file still formats (this time using SyntaxTree)

@vinistock vinistock requested a review from a team July 5, 2022 16:03
@vinistock vinistock self-assigned this Jul 5, 2022
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

We can test this by modifying the $LOAD_PATH like so:

$ irb
> require "rubocop"
true

$ irb
> $LOAD_PATH.delete_if {|path| path.start_with?(Gem.loaded_specs["rubocop"].full_gem_path) }
> require "rubocop"
LoadError: cannot load such file -- rubocop

(of course, we would need to recover the original $LOAD_PATH afterwards)

@vinistock vinistock force-pushed the vs/remove_rubocop_dependency branch from 7bd89fe to 57fbc26 Compare July 6, 2022 19:07
@vinistock vinistock requested a review from paracycle July 6, 2022 19:08
test/requests/formatting_test.rb Show resolved Hide resolved
@@ -20,7 +20,6 @@ Gem::Specification.new do |s|
s.require_paths = ["lib"]

s.add_dependency("language_server-protocol")
s.add_dependency("rubocop", ">= 1.0")
Copy link
Member

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/remove_rubocop_dependency branch from cb39605 to 081a6f7 Compare July 7, 2022 14:05
@vinistock
Copy link
Member Author

Renamed the method and also made unloading constants a bit more resilient. It was making the tests flaky when trying to remove constants, so I just wrapped it in a rescue.

@vinistock vinistock merged commit edef125 into main Jul 7, 2022
@vinistock vinistock deleted the vs/remove_rubocop_dependency branch July 7, 2022 14:42
@shopify-shipit shopify-shipit bot temporarily deployed to production July 7, 2022 19:06 Inactive
@shopify-shipit shopify-shipit bot temporarily deployed to 0-2-stable July 27, 2022 13:55 Inactive
andyw8 pushed a commit to andyw8/ruby-lsp that referenced this pull request Mar 2, 2024
…ypescript-eslint/eslint-plugin-5.33.0

Bump @typescript-eslint/eslint-plugin from 5.32.0 to 5.33.0
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.

Fallback to SyntaxTree formatting if RuboCop is not used by the application
3 participants