Skip to content

Explore plugin configs #498

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

Draft
sambostock wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Explore plugin configs #498

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5d617fc
[WIP] Explore plugin configs
sambostock Feb 14, 2023
9fd4371
Extract rubocop.infer.yml
sambostock Feb 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion .github/labeler.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1,8 @@
"config change": rubocop.yml
"config change": rubocop*.yml
"hq": rubocop.yml
Copy link
Contributor Author

@sambostock sambostock Feb 14, 2023

Choose a reason for hiding this comment

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

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

"graphql": rubocop.graphql.yml
"minitest": rubocop.minitest.yml
"performance": rubocop.performance.yml
"rails": rubocop.rails.yml
"rake": rubocop.rake.yml
"rspec": rubocop.rspec.yml
9 changes: 9 additions & 0 deletions Gemfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,12 @@ gem "diffy"
gem "minitest"
gem "pry-byebug"
gem "rake"

group :plugins do
Copy link
Contributor Author

@sambostock sambostock Feb 14, 2023

Choose a reason for hiding this comment

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

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.

gem "rubocop-graphql"
gem "rubocop-minitest"
gem "rubocop-performance"
gem "rubocop-rails"
gem "rubocop-rake"
gem "rubocop-rspec"
end
35 changes: 35 additions & 0 deletions Gemfile.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,18 @@ PATH
GEM
remote: https://rubygems.org/
specs:
activesupport (7.0.4.2)
concurrent-ruby (~> 1.0, >= 1.0.2)
i18n (>= 1.6, < 2)
minitest (>= 5.1)
tzinfo (~> 2.0)
ast (2.4.2)
byebug (11.1.3)
coderay (1.1.3)
concurrent-ruby (1.2.0)
diffy (3.4.2)
i18n (1.12.0)
concurrent-ruby (~> 1.0)
json (2.6.3)
method_source (1.0.0)
minitest (5.17.0)
Expand All @@ -23,6 +31,7 @@ GEM
pry-byebug (3.10.1)
byebug (~> 11.0)
pry (>= 0.13, < 0.15)
rack (3.0.4.1)
rainbow (3.1.1)
rake (13.0.6)
regexp_parser (2.6.2)
Expand All @@ -39,7 +48,27 @@ GEM
unicode-display_width (>= 2.4.0, < 3.0)
rubocop-ast (1.24.1)
parser (>= 3.1.1.0)
rubocop-capybara (2.17.0)
rubocop (~> 1.41)
rubocop-graphql (0.19.0)
rubocop (>= 0.87, < 2)
rubocop-minitest (0.27.0)
rubocop (>= 0.90, < 2.0)
rubocop-performance (1.16.0)
rubocop (>= 1.7.0, < 2.0)
rubocop-ast (>= 0.4.0)
rubocop-rails (2.17.4)
activesupport (>= 4.2.0)
rack (>= 1.1)
rubocop (>= 1.33.0, < 2.0)
rubocop-rake (0.6.0)
rubocop (~> 1.0)
rubocop-rspec (2.18.1)
rubocop (~> 1.33)
rubocop-capybara (~> 2.17)
ruby-progressbar (1.11.0)
tzinfo (2.0.6)
concurrent-ruby (~> 1.0)
unicode-display_width (2.4.2)

PLATFORMS
Expand All @@ -50,6 +79,12 @@ DEPENDENCIES
minitest
pry-byebug
rake
rubocop-graphql
rubocop-minitest
rubocop-performance
rubocop-rails
rubocop-rake
rubocop-rspec
rubocop-shopify!

BUNDLED WITH
Expand Down
Empty file added rubocop.graphql.yml
View file
Open in desktop
Empty file.
2 changes: 2 additions & 0 deletions rubocop.minitest.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Minitest:
Enabled: true
Comment on lines +1 to +2
Copy link
Contributor Author

@sambostock sambostock Feb 14, 2023

Choose a reason for hiding this comment

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

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...

Copy link
Contributor Author

@sambostock sambostock Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

I just went with what we use in Core. 🤷

Copy link
Member

@rafaelfranca rafaelfranca Oct 13, 2023

Choose a reason for hiding this comment

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

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

Empty file added rubocop.performance.yml
View file
Open in desktop
Empty file.
Empty file added rubocop.rails.yml
View file
Open in desktop
Empty file.
Empty file added rubocop.rake.yml
View file
Open in desktop
Empty file.
Empty file added rubocop.rspec.yml
View file
Open in desktop
Empty file.
16 changes: 16 additions & 0 deletions rubocop.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,22 @@ inherit_mode:
- Exclude
- Include

<%
# 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 %>
Copy link
Contributor Author

@sambostock sambostock Feb 14, 2023

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@sambostock sambostock Feb 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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


AllCops:
StyleGuideBaseURL: https://shopify.github.io/ruby-style-guide/

Expand Down
Loading