Add `RSpec/ClassCheck` cop #1357

r7kamura · 2022-08-05T10:33:33Z

It would be nice to have a new cop similar to the Style/ClassCheck cop.

https://docs.rubocop.org/rubocop/cops_style.html#styleclasscheck

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 created a new cop:

Added the new cop to config/default.yml.
The cop is configured as Enabled: pending in config/default.yml.
The cop is configured as Enabled: true in .rubocop.yml.
The cop documents examples of good and bad code.
The tests assert both that bad code is reported and that good code is not reported.
Set VersionAdded in default/config.yml to the next minor version.

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

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

ydah · 2022-08-05T11:12:57Z

lib/rubocop/cop/rspec/class_check.rb

+          format(
+            MSG,
+            current: node.method_name,
+            preferred: opposite_method_name(node.method_name)


How about the following?

Suggested change

preferred: opposite_method_name(node.method_name)

preferred: style

lib/rubocop/cop/rspec/class_check.rb

ydah · 2022-08-19T09:07:41Z

CI jobs that is failing now is being handled by the following PR Please wait a moment:

Fix InternalAffairs/NumblockHandler offenses #1362

bquorning

Nice addition to our collection of cops. I just had a couple of comments.

bquorning · 2022-08-25T12:58:49Z

.rubocop.yml

+RSpec/ClassCheck:
+  Enabled: true
+


Please move to line 110 or thereabouts.

bquorning · 2022-08-25T13:17:18Z

lib/rubocop/cop/rspec/class_check.rb

+        def autocorrect(corrector, node)
+          corrector.replace(node.loc.selector, style)
+        end
+
+        def format_message(node)
+          format(
+            MSG,
+            current: node.method_name,
+            preferred: style
+          )
+        end


I think I am directly contradicting @ydah when I say that I don’t like using style in line 63 and 70. The name of the style and the preferred literal code are incidentally identical, but they are two different concepts. While it’s not a big deal here (we probably won’t change the style names soon) it just doesn’t look right to me.

⬆️ This is just my opinion, not necessarily the truth 😄

Certainly, you have a point.

One of the reasons I chose to use style here is that it is better to calculate preferred method names based on style, rather than based on node. I think we can probably agree on this point. On top of that, if we want to make it explicit that style and method name are not the same concept, we should put a map from style to preferred method name, even if it seems a bit redundant.

I agree that the map would be the best approach. And having it map from style to preferred code (e.g. { be_a: 'be_a', be_kind_of: 'be_kind_of' }) does seem a bit redundant.

Darhazer

Since there is no restriction on the receiver, only on the method name, such a cop is prone to false positives, e.g. ResponseMock.be_a(sucessful_response) would be flagged. Even though there is a low chance for people to use such names, it won't hurt to check that the receiver is nil

Darhazer · 2022-09-05T14:59:11Z

spec/rubocop/cop/rspec/class_check_spec.rb

+
+  context 'when enforced style is be_kind_of and be_kind_of is used' do
+    let(:cop_config) do
+      { 'EnforcedStyle' => 'be_kind_of' }


I'd rather organize the spec in two top level contexts (by the enforced style) and then have examples for the usages; thus avoiding the repetition in cop_config

Darhazer · 2022-09-05T21:53:41Z

spec/rubocop/cop/rspec/class_check_spec.rb

+      let(:cop_config) do
+        { 'EnforcedStyle' => 'be_kind_of' }
+      end


You can move this config in the parent context, so you don't need to repeat it in the next ones

oh, I forgot to move them. Thank you! 👍

Darhazer

Thank you

.rubocop.yml

@@ -106,6 +106,8 @@ RSpec/BeNil:
  Enabled: true
 RSpec/ChangeByZero:
  Enabled: true
+RSpec/ClassCheck:
+  Enabled: true


ydah reviewed Aug 5, 2022

View reviewed changes

r7kamura force-pushed the feature/class-check branch from fb3d73b to d0e8f67 Compare August 5, 2022 11:54

Darhazer reviewed Aug 6, 2022

View reviewed changes

lib/rubocop/cop/rspec/class_check.rb Show resolved Hide resolved

r7kamura force-pushed the feature/class-check branch from 43b7e1b to 93cb479 Compare August 6, 2022 13:34

r7kamura force-pushed the feature/class-check branch from 93cb479 to 0b10d67 Compare August 19, 2022 09:04

r7kamura force-pushed the feature/class-check branch from 0b10d67 to 86e640e Compare August 25, 2022 11:04

Darhazer requested a review from bquorning August 25, 2022 12:40

bquorning reviewed Aug 25, 2022

View reviewed changes

r7kamura force-pushed the feature/class-check branch 2 times, most recently from 601fb69 to cea3ae5 Compare August 25, 2022 14:22

r7kamura force-pushed the feature/class-check branch from cea3ae5 to 560982f Compare September 1, 2022 21:14

bquorning approved these changes Sep 2, 2022

View reviewed changes

bquorning requested a review from Darhazer September 2, 2022 06:56

Darhazer reviewed Sep 5, 2022

View reviewed changes

r7kamura force-pushed the feature/class-check branch from 560982f to 6544044 Compare September 5, 2022 21:17

Darhazer reviewed Sep 5, 2022

View reviewed changes

Add RSpec/ClassCheck cop

d9f1335

r7kamura force-pushed the feature/class-check branch from 6544044 to d9f1335 Compare September 5, 2022 22:00

Darhazer approved these changes Sep 6, 2022

View reviewed changes

Darhazer merged commit d272e8e into rubocop:master Sep 7, 2022

r7kamura deleted the feature/class-check branch September 7, 2022 11:17

pirj reviewed Nov 4, 2022

View reviewed changes

.rubocop.yml

@@ -106,6 +106,8 @@ RSpec/BeNil:

Enabled: true

RSpec/ChangeByZero:

Enabled: true

RSpec/ClassCheck:

Enabled: true

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `RSpec/ClassCheck` cop #1357

Add `RSpec/ClassCheck` cop #1357

r7kamura commented Aug 5, 2022

ydah Aug 5, 2022

ydah commented Aug 19, 2022 •

edited

bquorning left a comment

bquorning Aug 25, 2022

bquorning Aug 25, 2022

r7kamura Aug 25, 2022

bquorning Aug 25, 2022

Darhazer left a comment

Darhazer Sep 5, 2022

Darhazer Sep 5, 2022

r7kamura Sep 5, 2022

Darhazer left a comment

This comment was marked as resolved.

This comment was marked as resolved.

	preferred: opposite_method_name(node.method_name)
	preferred: style

Add RSpec/ClassCheck cop #1357

Add RSpec/ClassCheck cop #1357

Conversation

r7kamura commented Aug 5, 2022

Choose a reason for hiding this comment

ydah commented Aug 19, 2022 • edited

bquorning left a 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

Darhazer left a 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

Darhazer left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Add `RSpec/ClassCheck` cop #1357

Add `RSpec/ClassCheck` cop #1357

ydah commented Aug 19, 2022 •

edited