Skip to content
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.

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

Add new RSpec/RemoveConst cop #1749

Merged
merged 1 commit into from
Dec 14, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions .rubocop.yml
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,8 @@ RSpec/RedundantAround:
Enabled: true
RSpec/RedundantPredicateMatcher:
Enabled: true
RSpec/RemoveConst:
Enabled: true
RSpec/SkipBlockInsideExample:
Enabled: true
RSpec/SortMetadata:
Expand Down
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

- Add new `RSpec/RedundantPredicateMatcher` cop. ([@ydah])
- Add support for correcting "it will" (future tense) for `RSpec/ExampleWording`. ([@jdufresne])
- Add new `RSpec/RemoveConst` cop. ([@swelther])

## 2.25.0 (2023-10-27)

Expand Down Expand Up @@ -913,6 +914,7 @@ Compatibility release so users can upgrade RuboCop to 0.51.0. No new features.
[@smcgivern]: https://github.com/smcgivern
[@splattael]: https://github.com/splattael
[@stephannv]: https://github.com/stephannv
[@swelther]: https://github.com/swelther
[@t3h2mas]: https://github.com/t3h2mas
[@tdeo]: https://github.com/tdeo
[@tejasbubane]: https://github.com/tejasbubane
Expand Down
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -777,6 +777,12 @@ RSpec/RedundantPredicateMatcher:
VersionAdded: "<<next>>"
Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RedundantPredicateMatcher

RSpec/RemoveConst:
Description: Checks that `remove_const` is not used in specs.
Enabled: pending
VersionAdded: "<<next>>"
Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RemoveConst

RSpec/RepeatedDescription:
Description: Check for repeated description strings in example groups.
Enabled: true
Expand Down
1 change: 1 addition & 0 deletions docs/modules/ROOT/pages/cops.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
* xref:cops_rspec.adoc#rspecreceivenever[RSpec/ReceiveNever]
* xref:cops_rspec.adoc#rspecredundantaround[RSpec/RedundantAround]
* xref:cops_rspec.adoc#rspecredundantpredicatematcher[RSpec/RedundantPredicateMatcher]
* xref:cops_rspec.adoc#rspecremoveconst[RSpec/RemoveConst]
* xref:cops_rspec.adoc#rspecrepeateddescription[RSpec/RepeatedDescription]
* xref:cops_rspec.adoc#rspecrepeatedexample[RSpec/RepeatedExample]
* xref:cops_rspec.adoc#rspecrepeatedexamplegroupbody[RSpec/RepeatedExampleGroupBody]
Expand Down
32 changes: 32 additions & 0 deletions docs/modules/ROOT/pages/cops_rspec.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -4525,6 +4525,38 @@ expect(foo).not_to include(bar)

* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RedundantPredicateMatcher

== RSpec/RemoveConst

|===
| Enabled by default | Safe | Supports autocorrection | Version Added | Version Changed

| Pending
| Yes
| No
| <<next>>
| -
|===

Checks that `remove_const` is not used in specs.

=== Examples

[source,ruby]
----
# bad
it 'does something' do
Object.send(:remove_const, :SomeConstant)
end
before do
SomeClass.send(:remove_const, :SomeConstant)
end
----

=== References

* https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RemoveConst

== RSpec/RepeatedDescription

|===
Expand Down
40 changes: 40 additions & 0 deletions lib/rubocop/cop/rspec/remove_const.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
# frozen_string_literal: true

module RuboCop
module Cop
module RSpec
# Checks that `remove_const` is not used in specs.
#
# @example
# # bad
# it 'does something' do
# Object.send(:remove_const, :SomeConstant)
# end
#
pirj marked this conversation as resolved.
Show resolved Hide resolved
# before do
# SomeClass.send(:remove_const, :SomeConstant)
# end
#
Copy link
Member

@pirj pirj Dec 4, 2023

Choose a reason for hiding this comment

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

Does it make sense to add an exqmple with stub_const as good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question 🤔 We had some spec helper that used Object.remove_const and Object.const_set to change a global config in our specs. This caused several issues in our CI so I added this as an example. But I'm no longer sure if this is a good example in the wild?

class RemoveConst < Base
include RuboCop::RSpec::Language
extend RuboCop::RSpec::Language::NodePattern

MSG = 'Do not use remove_const in specs. ' \
'Consider using e.g. `stub_const`.'
RESTRICT_ON_SEND = %i[send __send__].freeze

bquorning marked this conversation as resolved.
Show resolved Hide resolved
# @!method remove_const(node)
def_node_matcher :remove_const, <<~PATTERN
(send _ {:send | :__send__} (sym :remove_const) _)
Copy link
Collaborator

Choose a reason for hiding this comment

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

More a comment for @pirj than for @swelther:

I don’t know that the | is necessary, but I found that any of these 3 patterns will let the current specs pass:

  1. (send _ {:send | :__send__} (sym :remove_const) _)
  2. (send _ {:send :__send__} (sym :remove_const) _)
  3. (send _ {:send __send__} (sym :remove_const) _)

Curiously, (send _ {:send | __send__} (sym :remove_const) _) will not work.

Copy link
Member

Choose a reason for hiding this comment

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

Ha, another node pattern related gotcha gotcha in the same PR!

Agree, | is optional, but it actually helped finding a match-all node.
__send__ is a named “any node”, basically the same as _. https://docs.rubocop.org/rubocop-ast/node_pattern.html had an example with (int _value). I think we used that in our cops, too, but it’s not documented.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

so, do I need to change something? Sorry, I'm a bit lost here 😅

Copy link
Member

Choose a reason for hiding this comment

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

You may want to remove the | as it’s redundant, or to keep it for failsafe. Up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the | syntax more because it looks like regex and might be easier to understand. Would like to keep it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the docs I found this bit:

If all the branches have a single term, you can omit the |, so {int | float} can be simplified to {int float}.

So, apparently it’s valid syntax – I just hadn’t seen it before 😳 Let’s just leave the | in.

PATTERN

# Check for offenses
def on_send(node)
remove_const(node) do
add_offense(node)
end
end
end
end
end
end
1 change: 1 addition & 0 deletions lib/rubocop/cop/rspec_cops.rb
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@
require_relative 'rspec/receive_never'
require_relative 'rspec/redundant_around'
require_relative 'rspec/redundant_predicate_matcher'
require_relative 'rspec/remove_const'
require_relative 'rspec/repeated_description'
require_relative 'rspec/repeated_example'
require_relative 'rspec/repeated_example_group_body'
Expand Down
28 changes: 28 additions & 0 deletions spec/rubocop/cop/rspec/remove_const_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::RSpec::RemoveConst do
it 'detects the `remove_const` usage' do
expect_offense(<<-RUBY)
it 'does something' do
Object.send(:remove_const, :SomeConstant)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use remove_const in specs. Consider using e.g. `stub_const`.
end
RUBY

expect_offense(<<-RUBY)
before do
SomeClass.send(:remove_const, :SomeConstant)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use remove_const in specs. Consider using e.g. `stub_const`.
end
RUBY
end

it 'detects the `remove_const` usage when using `__send__`' do
expect_offense(<<-RUBY)
it 'does something' do
NiceClass.__send__(:remove_const, :SomeConstant)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Do not use remove_const in specs. Consider using e.g. `stub_const`.
end
RUBY
end
end