rubocop-rspec icon indicating copy to clipboard operation
rubocop-rspec copied to clipboard

Extract part of project to rubocop-extension

Open backus opened this issue 8 years ago • 3 comments

The Inject.defaults! code is duplicated across many third party gems

This code (plus a few other pieces of rubocop-rspec) would probably benefit from being extracted to a gem that unifies logic for making a rubocop extension. This would also make the eventual transition to https://github.com/bbatsov/rubocop/issues/3414 easier.

backus avatar Nov 28 '16 22:11 backus

It looks like RuboCop RSpec’s Inject code has become the standard way of adding 3rd party cops to RuboCop.

  • https://github.com/rubocop-hq/rubocop-rubycw/blob/da0c25d3bce406ed19a1066e5a2c91f05165736c/lib/rubocop/rubycw/inject.rb#L10-L17
  • https://github.com/rubocop-hq/rubocop-rspec/blob/fc8422ae65907721a0be36167fbdcfff0159a414/lib/rubocop/rspec/inject.rb#L8-L17
  • https://github.com/rubocop-hq/rubocop-performance/blob/eb26b4ae942704be1dd2b5c14ef613692c1747d1/lib/rubocop/performance/inject.rb#L8-L15
  • https://github.com/rubocop-hq/rubocop-md/blob/6e074498a0949fcc20b3696a4f9f07a42506d717/lib/rubocop/markdown/rubocop_ext.rb#L25-L32
  • https://github.com/rubocop-hq/rubocop-rails/blob/41a0401a2144cdcac76ba08b21a7fa13bb09a5fc/lib/rubocop/rails/inject.rb#L8-L15
  • https://github.com/rubocop-hq/rubocop-minitest/blob/5e488f0978aabd26fd2e72eb174ae83f0e3a7851/lib/rubocop/minitest/inject.rb#L8-L15
  • https://github.com/rubocop-hq/rubocop-rake/blob/f4d49f0eeb4c52eb8bef055aefb125cfd1e57d5d/lib/rubocop/rake/inject.rb#L10-L17

We can do better than this!

The simplest approach I can think of, is to copy our code into RuboCop::ConfigLoader, e.g.

RuboCop
  class ConfigLoader
    class << self
      def load_plugin!(path)
        path = path.to_s
        hash = load_yaml_configuration(path)
        config = Config.new(hash, path)
        puts "Plugin configuration from #{path}" if debug?
        config = merge_with_default(config, path)
        self.default_configuration = config
      end
    end
  end
end

In lib/rubocop_rspec.rb we can delete the RuboCop::RSpec::Inject.defaults! call and instead do

RuboCop::ConfigLoader.load_plugin!(
  File.expand_path('../config/default.yml', __dir__)
)

I know nothing about the internals of ConfigLoader, so maybe there is some code duplication or some obvious refactoring that can be done. But what do you think of this API? Do we need to register anything else? The RuboCop::RSpec namespace? Or is passing a path (as a string) enough?

/cc @Darhazer @pirj @bbatsov

bquorning avatar Nov 06 '20 22:11 bquorning

Adding a method to RuboCop for extensions to load ownself's config looks strange to me overall. Shouldn't RuboCop decide on its own which config to load depending on require in .rubocop.yml? There's not much harm in adding e.g. rubocop-rspec's configuration when rubocop-rspec is loaded by Bundler but not required from .rubocop.yml, but it is useless.

I don't know about ConfigLoader. From a newbie's point of view I would expect it to load extensions' configs, especially ConfigLoaderResolver#resolve_requires goes as far as require gems.

pirj avatar Nov 07 '20 19:11 pirj

I might be mistaken, but it seems that this scheme circumvents the ability of extensions to require other extensions without explicitly require'ing them from Ruby code. When extension's config/default.yml gets loaded, its requires are ignored.

It's not a problem with the current Inject code, but that would load rubocop-factory_bot code when running rubocop-rspec specs, while it's only a sidecar, and we only need it when running rubocop from the application that specifies rubocop-rspec in its .rubocop.yml.

pirj avatar Nov 07 '20 19:11 pirj

Fixed in https://github.com/rubocop/rubocop/pull/11265 There's https://github.com/rubocop/rubocop-rspec/pull/1522 to use it, but it broke after capybara extraction, needs some debugging. I'll close as completed.

pirj avatar May 07 '23 21:05 pirj