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

Detect RuboCop as the formatter when it is only a transitive depenency #2126

Merged
merged 1 commit into from
Jun 11, 2024

Conversation

Earlopain
Copy link
Contributor

Motivation

Closes #1198

Implementation

I've followed what is outlined in #1198 (comment). GlobalState now has the workspace uri so the implementation is relativly straightforward. I'm slightly deviating by also checking that rubocop is a transitive dependency, seems like a good thing to do.

I've moved file checking into its own method. This is purely for tests as mocha doesn't make it possible to stub a method with only a specific set of arguments. File.stubs(:exist?).with("foo") disallows all other invocations of that method.

Automated Tests

Added some for the combinations.

Manual Tests

That's a bit more complicated. I'm not aware of a public gem that provides a rubocop config that also doesn't start with rubocop. I created a local shim gem that just contains a rubocop.yml/gemspec and tested out the various scenarios.

@Earlopain Earlopain requested a review from a team as a code owner June 4, 2024 10:44
@Earlopain Earlopain requested review from andyw8 and vinistock June 4, 2024 10:44
@Earlopain Earlopain force-pushed the rubocop-transitive-dependency branch from cb96e8a to 6ce1027 Compare June 4, 2024 10:46
@vinistock vinistock added bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes labels Jun 7, 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.

This is great! Just a minor change in logic and we should be good to go

lib/ruby_lsp/global_state.rb Outdated Show resolved Hide resolved
@Earlopain Earlopain force-pushed the rubocop-transitive-dependency branch 2 times, most recently from cba61c1 to 8fb12cc Compare June 11, 2024 14:19
@Earlopain Earlopain force-pushed the rubocop-transitive-dependency branch from 8fb12cc to 02cefd4 Compare June 11, 2024 16:23
@Earlopain
Copy link
Contributor Author

This is good to go now. There was some issue with how I was stubbing bundler. Apparently this isn't how you're supposed to use structs (at least in Ruby 3.1). You must pas's keyword_init for this to properly work:

BundlerSpec = Struct.new(:name)
BundlerSpec.new(name: "whatever")

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 7664980 into Shopify:main Jun 11, 2024
18 checks passed
@Earlopain Earlopain deleted the rubocop-transitive-dependency branch June 11, 2024 16:50
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.

Formatting not working with neovim when rubocop is a transient dependency
2 participants