Remove relevance detection code #1063

pirj · 2020-10-27T22:00:08Z

We have had problems with it:

it was slow and memory-hungry
it uses a non-standard extension of RuboCop's config (Patterns), while a comparable Include option exists
department-specific (e.g. FactoryBot) config was ignored
for the same department (i.e. FactoryBot) files to inspect are different for different cops

Close #967
Close #1062
Fixes #1044

It's all interconnected in this PR, here's the path I had to go through:

Remove the code that checks file relevance
Replace Patterns with Include
Adjust config/default.yml
Adjust specs, specifying the file for other departments
Tweak config shared example to respect department settings
Adjust config formatter
Fix spec that failed to detect offences in an unrelated file

TODOs

extract 7 into a separate PR (extracted to Refactor a spec #1064)
adjust Add v2 update docs #1013 to mention Patterns -> Include migration (added a TODO there)

Before submitting the PR make sure the following are checked:

Feature branch is up-to-date with master (if not - rebase it).
Squashed related commits together.
Added tests.
Updated documentation.
Added an entry to the CHANGELOG.md if the new code introduces user-observable changes.
The build (bundle exec rake) passes (be sure to run this locally, since it may produce updated documentation that you will need to commit).

If you have modified an existing cop's configuration options:

Set VersionChanged in config/default.yml to the next major version.

pirj · 2020-10-27T22:04:16Z

@fatkodima Please take a look, what do you think about memory consumption? Is this faster, or worse than previously?
For some reason, I can't invite you as a reviewer.

fatkodima · 2020-10-27T23:08:56Z

For some reason, I can't invite you as a reviewer.

I think that's because I'm not a member of a rubocop organization.

Just measured - memory is good, good job! 👏 👏

pirj · 2020-10-27T22:55:21Z

.rubocop_todo.yml

+
+RSpec/ExampleLength:
+  Exclude:
+    - spec/rubocop/cop/rspec/factory_bot/attribute_defined_statically_spec.rb


It was a single it_behaves_like with good_code and bad_code defined on example group level. Now, when it's an example, it has overflown the allowed margin.

pirj · 2020-10-27T22:59:07Z

config/default.yml

@@ -1,14 +1,9 @@
 ---
-AllCops:


The reason I've dropped AllCops is that it's never mentioned in the docs it's a preferred way to configure cops in departments. It works quite fine on the root level. https://docs.rubocop.org/rubocop/1.0/configuration.html
Also, I think there was some complication where for_badge didn't work properly with configuration options defined inside AllCops.

pirj · 2020-10-27T22:59:49Z

config/default.yml

@@ -241,6 +236,9 @@ RSpec/ExpectOutput:
 RSpec/FilePath:
  Description: Checks that spec file paths are consistent and well-formed.
  Enabled: true
+  Include:
+  - "**/*_spec*rb*"


This is due to file name detection, e.g. some_spec.rb.rb and some_specorb.

pirj · 2020-10-27T23:01:47Z

config/default.yml

-    - features/support/factories/**/*.rb
+
+RSpec:
+  Include:


This behavior of Include (overriding default.yml) was introduced in 0.56.0 via rubocop/rubocop#5882. This change allows people to include/exclude precisely what they need to, without the defaults getting in the way.

pirj · 2020-10-27T23:02:17Z

config/default.yml

+  Include:
+  - spec/factories.rb
+  - spec/factories/**/*.rb
+  - features/support/factories/**/*.rb


This cop should only inspect factory definitions.

pirj · 2020-10-27T23:07:01Z

spec/rubocop/cop/rspec/factory_bot/attribute_defined_statically_spec.rb

@@ -1,22 +1,57 @@
 # frozen_string_literal: true

 RSpec.describe RuboCop::Cop::RSpec::FactoryBot::AttributeDefinedStatically do
+  def inspected_source_filename


Our overloaded expect_offense/expect_no_offenses use this to pass the file name to super.

This tells the cop that it inspects a relevant source.

pirj · 2020-10-27T23:08:40Z

spec/rubocop/cop/rspec/factory_bot/attribute_defined_statically_spec.rb

-    end
-  RUBY
-
-  include_examples 'autocorrect', bad, corrected


autocorrect shared example does not pass the correct file path to expect_offense, and thus the cop considers the file irrelevant and skips it, failing the example.

spec/rubocop/cop/rspec/factory_bot/attribute_defined_statically_spec.rb

pirj · 2020-10-27T23:11:21Z

spec/rubocop/cop/rspec/instance_variable_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true

-RSpec.describe RuboCop::Cop::RSpec::InstanceVariable do
+RSpec.describe RuboCop::Cop::RSpec::InstanceVariable, :config do


Because it needs the default config, specifically Include, otherwise

expect_no_offenses(<<-RUBY, 'lib/source_code.rb')

fails.

pirj · 2020-10-27T23:12:07Z

spec/support/config.rb

+    department_name = cop_class.badge.department.to_s
+    # By default, `RSpec/Include: ['**/*_spec.rb', '**/spec/**/*']`
+    department_configuration = RuboCop::ConfigLoader
+      .default_configuration
+      .for_department(department_name)


I hope to submit this upstream.

pirj · 2020-10-27T23:14:09Z

memory is good, good job!

@fatkodima Thanks! 🙌

not a member of a rubocop organization.

Just not yet. Keep up the good work!

pirj · 2020-10-28T09:55:06Z

@sl4vr Appreciate it if you could take a look.
I have the impression that this change will make #956 simpler. WDYT?

pirj · 2020-10-28T10:01:52Z

Rebased. It's down from +194 −162 to +113 −106 now.
@bquorning please take a look, but no rush.

pirj · 2020-10-28T10:16:11Z

spec/support/expect_offense.rb

    super
  end

-  def expect_no_offenses(source, filename = DEFAULT_FILENAME) # rubocop:disable Lint/UselessMethodDefinition


@tejasbubane Removed it 🤷

bquorning

This is looking real good. rubocop/rubocop#5882 is an amazing find, and I hope it works as well as it seems 👍

docs/modules/ROOT/pages/usage.adoc

lib/rubocop/rspec/config_formatter.rb

spec/rubocop/cop/rspec/base_spec.rb

We have had problems with it: - it was slow - it is a non-standard extension of RuboCop's config - Include option exists - department-specific (e.g. FactoryBot) config was ignored

pirj · 2020-10-28T22:48:50Z

@bquorning @Darhazer Please merge at will. Addressed your concerns.

pirj self-assigned this Oct 27, 2020

pirj requested review from bquorning and Darhazer October 27, 2020 22:00

This was referenced Oct 27, 2020

Release 2.0 #1055

Merged

Add v2 update docs #1013

Merged

Refactor a spec #1064

Merged

pirj added this to the 2.0 milestone Oct 27, 2020

pirj marked this pull request as ready for review October 27, 2020 22:52

pirj commented Oct 27, 2020

View reviewed changes

pirj mentioned this pull request Oct 28, 2020

RSpec aliases runtime matchers #956

Merged

16 tasks

pirj force-pushed the remove-relevance-detection-code branch from 561d04e to 676b2f4 Compare October 28, 2020 09:59

pirj commented Oct 28, 2020

View reviewed changes

bquorning reviewed Oct 28, 2020

View reviewed changes

docs/modules/ROOT/pages/usage.adoc Show resolved Hide resolved

lib/rubocop/rspec/config_formatter.rb Outdated Show resolved Hide resolved

spec/rubocop/cop/rspec/base_spec.rb Outdated Show resolved Hide resolved

pirj force-pushed the remove-relevance-detection-code branch from c2105b0 to bcb058c Compare October 28, 2020 22:38

Remove relevance detection code

4920879

We have had problems with it: - it was slow - it is a non-standard extension of RuboCop's config - Include option exists - department-specific (e.g. FactoryBot) config was ignored

pirj force-pushed the remove-relevance-detection-code branch from 7fb58c3 to 4920879 Compare October 28, 2020 22:44

bquorning approved these changes Oct 29, 2020

View reviewed changes

bquorning merged commit 2cd7639 into release-2.0 Oct 29, 2020

bquorning deleted the remove-relevance-detection-code branch October 29, 2020 11:58

Darhazer approved these changes Oct 29, 2020

View reviewed changes

This was referenced Oct 29, 2020

Optimize Base#relevant_rubocop_rspec_file? #967

Closed

Optimize Base#relevant_file? #1062

Closed

Consider extracting FactoryBot cops to a separate gem #1072

Closed

pirj mentioned this pull request Nov 6, 2020

FactoryBot/AttributeDefinedStatically doesn't work well #1044

Closed

bquorning mentioned this pull request Nov 14, 2020

Remove seemingly unnecessary spec config file #1094

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove relevance detection code #1063

Remove relevance detection code #1063

pirj commented Oct 27, 2020 •

edited

Loading

pirj commented Oct 27, 2020

fatkodima commented Oct 27, 2020

pirj Oct 27, 2020

pirj Oct 27, 2020

pirj Oct 27, 2020

pirj Oct 27, 2020

pirj Oct 27, 2020

pirj Oct 27, 2020

pirj Oct 27, 2020

pirj Oct 27, 2020

pirj Oct 27, 2020

pirj commented Oct 27, 2020

pirj commented Oct 28, 2020

pirj commented Oct 28, 2020

pirj Oct 28, 2020

bquorning left a comment

pirj commented Oct 28, 2020

Remove relevance detection code #1063

Remove relevance detection code #1063

Conversation

pirj commented Oct 27, 2020 • edited Loading

TODOs

pirj commented Oct 27, 2020

fatkodima commented Oct 27, 2020

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

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

pirj commented Oct 27, 2020

pirj commented Oct 28, 2020

pirj commented Oct 28, 2020

Choose a reason for hiding this comment

bquorning left a comment

Choose a reason for hiding this comment

pirj commented Oct 28, 2020

pirj commented Oct 27, 2020 •

edited

Loading