Explore plugin configs #498

sambostock · 2023-02-14T04:09:23Z

This PR explores the idea of us also providing configs for other RuboCop plugins. Currently teams at Shopify configure these independently, but maybe we want to take a stance on some of the cops.

Warning
Work in progress.

RuboCop config requires only the RuboCop plugins whose gemspec is loaded
Each plugin has a corresponding file of defaults which are inherited
Labels are attached automatically to PRs touching each config file

TBD:

Should we have a minimal config? Should we keep the existing config as is, and instead add an "everything.yml" config?
Should we test compatibility with just RuboCop and no plugins?
What cops to enable? Maybe disallowing "pending" shouldn't apply to plugins, so we don't break consumers?

- RuboCop config requires only the RuboCop plugins whose gemspec is loaded - Each plugin has a corresponding file of defaults which are inherited - Labels are attached automatically to PRs touching each config file TBD: - Should we have a minimal config? Should we keep the existing config as is, and instead add an "everything.yml" config? - Should we test compatibility with _just_ RuboCop and no plugins? - What cops to enable? Maybe disallowing "pending" shouldn't apply to plugins, so we don't break consumers?

sambostock · 2023-02-14T04:09:55Z

.github/labeler.yml

@@ -1 +1,8 @@
-"config change": rubocop.yml
+"config change": rubocop*.yml
+"hq": rubocop.yml


Wasn't sure of a good label name, so I went with the "cop" analogy of headquarters. 🤷

sambostock · 2023-02-14T04:10:51Z

Gemfile

@@ -8,3 +8,12 @@ gem "diffy"
 gem "minitest"
 gem "pry-byebug"
 gem "rake"
+
+group :plugins do


I think we could use this to skip installing these in one of our CI runs, so we can ensure everything still works for consumers only using RuboCop.

sambostock · 2023-02-14T04:12:20Z

rubocop.minitest.yml

+Minitest:
+  Enabled: true


I haven't bothered filling out most of these files, but presumably they'd look like:

PluginName: Enabled: true # ...config for cops where we want to diverge from the default...

We could alternatively call these after the gem names (e.g. rubocop-minitest.yml).

I just went with what we use in Core. 🤷

I prefer to not use defaults. Let's always curate cops.

sambostock · 2023-02-14T04:19:10Z

rubocop.yml

+<%
+  # Identify installed RuboCop plugins to configure
+  plugin_configs = {
+    "rubocop-graphql" => "rubocop.graphql.yml",
+    "rubocop-minitest" => "rubocop.minitest.yml",
+    "rubocop-performance" => "rubocop.performance.yml",
+    "rubocop-rails" => "rubocop.rails.yml",
+    "rubocop-rake" => "rubocop.graphql.yml",
+    "rubocop-rspec" => "rubocop.rspec.yml",
+  }.select { |plugin_name, _| Gem.loaded_specs.include?(plugin_name) }
+%>
+
+require: <%= plugin_configs.keys.to_json %>
+
+inherit_from: <%= plugin_configs.values.to_json %>


This is where the magic happens. 🪄 ✨ 👮‍♂️

Are there reasons why this would be a bad idea?

Pros

Allows us to inject config without forcing the dependency on any plugins, or knowing anything fancy about the host app.

Removes the need to separate gems (e.g. rubocop-rails-shopify or whatever).

Removes the need for consumers to opt-into the configs themselves. They can simply follow the suggestions from Rubocop, and as soon as they add a plugin it will "magically" be configured.

Cons

Doesn't allow us to enforce the version of the plugins.

This might get tricky as cops are added/removed.

We would probably have to fallback to enforcing the version in ERB, which isn't ideal.

Could there be cases where it's unsafe to enable these?

We could move the magic into a special file (e.g. infer-rubocop.yml), and keep this file simple, but that would mean consumers would have to know to switch.

Maybe it's sufficient to require consumers to simply override the config if it isn't compatible.

I've tentatively moved this into rubocop.infer.yml, since this repo itself ran into inapplicable cops. Although that may be a special case, so we can always put it back. If we went with this separate file, then it would mean consumers could opt into the magic

inherit_gem: rubocop-shopify: rubocop.infer.yml # or perhaps we call it `rubocop.shopify.yml`

or stick with the current minimal approach:

inherit_gem rubocop-shopify: rubocop.yml

or take a custom approach

inherit_gem rubocop-shopify: - rubocop.yml - rubocop.rails.yml

rafaelfranca · 2023-10-13T16:09:07Z

rubocop.infer.yml

+    "rubocop-rails" => "rubocop.rails.yml",
+    "rubocop-rake" => "rubocop.graphql.yml",
+    "rubocop-rspec" => "rubocop.rspec.yml",
+  }.select { |plugin_name, _| Gem.loaded_specs.include?(plugin_name) }


The tricky part is that this checks if the gem is installed in the computer, not in the bundler right?

Yes. The way it would work is that if a consumer has

# .rubocop.yml inherit_gem: rubocop-shopify: rubocop.infer.yml

then when they run rubocop it will look at their installed gems, and inherit from the applicable configs.

But if the app doesn't have rubocop-minitest for example but the person has it installed globally we should not use it.

We should be using what is on the Gemfile.lock of the application, not what is on the gem paths.

Yes, good point. I should have said "in their Gemfile" not "installed". I just confirmed Gem.loaded_specs handles this the way we want:

% cat Gemfile source 'https://rubygems.org' % bundle exec ruby -e 'puts Gem.loaded_specs.key?("rubocop-minitest")' false % gem install rubocop-minitest Successfully installed rubocop-minitest-0.32.2 Parsing documentation for rubocop-minitest-0.32.2 Installing ri documentation for rubocop-minitest-0.32.2 Done installing documentation for rubocop-minitest after 0 seconds 1 gem installed % bundle exec ruby -e 'puts Gem.loaded_specs.key?("rubocop-minitest")' false % bundle add rubocop-minitest > /dev/null && cat Gemfile source 'https://rubygems.org' gem "rubocop-minitest", "~> 0.32.2" % bundle exec ruby -e 'puts Gem.loaded_specs.key?("rubocop-minitest")' true % bundle exec ruby -e 'puts Gem.loaded_specs.key?("rubocop")' true

sambostock requested a review from rafaelfranca February 14, 2023 04:09

github-actions bot added the config change Changes the Rubocop config by enabling, disabling, or reconfiguring one or many cops label Feb 14, 2023

sambostock commented Feb 14, 2023

View reviewed changes

github-actions bot removed the config change Changes the Rubocop config by enabling, disabling, or reconfiguring one or many cops label Feb 14, 2023

Extract rubocop.infer.yml

9fd4371

sambostock force-pushed the plugins branch from e3d503a to 9fd4371 Compare February 14, 2023 04:50

rafaelfranca reviewed Oct 13, 2023

View reviewed changes

andyw8 mentioned this pull request Dec 13, 2023

Enable Minitest/AssertPredicate cop Shopify/ruby-lsp#1249

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Explore plugin configs #498

Explore plugin configs #498

sambostock commented Feb 14, 2023

sambostock Feb 14, 2023

sambostock Feb 14, 2023

sambostock Feb 14, 2023

sambostock Feb 14, 2023 •

edited

rafaelfranca Oct 13, 2023

sambostock Feb 14, 2023

sambostock Feb 14, 2023 •

edited

rafaelfranca Oct 13, 2023

sambostock Oct 13, 2023

rafaelfranca Oct 13, 2023

sambostock Oct 13, 2023

Explore plugin configs #498

Are you sure you want to change the base?

Explore plugin configs #498

Conversation

sambostock commented Feb 14, 2023

TBD:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sambostock Feb 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pros

Cons

sambostock Feb 14, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sambostock Feb 14, 2023 •

edited

sambostock Feb 14, 2023 •

edited