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 RedundantContext cop #2020

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
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 @@ -255,3 +255,5 @@ Style/YAMLFileRead: {Enabled: true}
#
RSpec/StringAsInstanceDoubleConstant:
Enabled: true
RSpec/RedundantContext:
Enabled: true
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Master (Unreleased)

- Add new `RSpec/RedundantContext` cop. ([@tejasbubane])

## 3.3.0 (2024-12-12)

- Deprecate `top_level_group?` method from `TopLevelGroup` mixin as all of its callers were intentionally removed from `Rubocop/RSpec`. ([@corsonknowles])
Expand Down
6 changes: 6 additions & 0 deletions config/default.yml
Original file line number Diff line number Diff line change
Expand Up @@ -794,6 +794,12 @@ RSpec/RedundantAround:
VersionAdded: '2.19'
Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RedundantAround

RSpec/RedundantContext:
Description: Detect redundant `context` hook.
Enabled: pending
Copy link
Member

Choose a reason for hiding this comment

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

I’m usually strongly against disabling cops, but this one, even though certainly serving some projects that strive to enforce this style, isn’t a good fit for the majority.

I support what @koic and @bquorning said.

VersionAdded: "<<next>>"
Reference: https://www.rubydoc.info/gems/rubocop-rspec/RuboCop/Cop/RSpec/RedundantContext

RSpec/RedundantPredicateMatcher:
Description: Checks for redundant predicate matcher.
Enabled: true
Expand Down
39 changes: 39 additions & 0 deletions lib/rubocop/cop/rspec/redundant_context.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true

module RuboCop
module Cop
module RSpec
# Detect redundant `context` hook.
#
# @example
# # bad
# context 'when condition' do
# it 'tests something' do
# end
# end
#
# # good
# it 'tests something when condition' do
# end
#
class RedundantContext < Base
MSG = 'Redundant context with single example.'

# @!method redundant_context?(node)
def_node_matcher :redundant_context?, <<~PATTERN
(block
(send #rspec? :context _)
Copy link
Member

Choose a reason for hiding this comment

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

Just specifically context and it?
I’dd love to have at least balancing examples for describe/example aliases if it’s a deliberate design choice.

Copy link
Member

Choose a reason for hiding this comment

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

The _ suggests that it only has one anything argument, with an assumption that it’s a docstring. But what if there’s a secondary string argument, or symbolic/hash metadata, too?

_
(block (send _ :it ...) ...))
Copy link
Member

@pirj pirj Jan 8, 2025

Choose a reason for hiding this comment

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

Cosmetic: I think the receiver should be nil here, not _

PATTERN

def on_block(node)
return unless redundant_context?(node)

add_offense(node)
end
alias on_numblock on_block
Copy link
Member

Choose a reason for hiding this comment

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

Will a numblock match with the current node pattern?

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 @@ -81,6 +81,7 @@
require_relative 'rspec/receive_messages'
require_relative 'rspec/receive_never'
require_relative 'rspec/redundant_around'
require_relative 'rspec/redundant_context'
require_relative 'rspec/redundant_predicate_matcher'
require_relative 'rspec/remove_const'
require_relative 'rspec/repeated_description'
Expand Down
25 changes: 25 additions & 0 deletions spec/rubocop/cop/rspec/redundant_context_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::RSpec::RedundantContext do
it 'registers an offense when single example inside context' do
expect_offense(<<~RUBY)
context 'when condition' do
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Redundant context with single example.
it 'does something' do
end
end
RUBY
end

it 'does not register offense when multiple examples inside context' do
Copy link
Member

Choose a reason for hiding this comment

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

It is more fair to say “when the example is the only statement inside the example group. Eg another it_behaves_like or a method def would cause the cop to ignore the group.

Are there any cases with more than one statement (but one example) that we still want to flag/correct?
If not - just ignore this note, just make the description more generic, and maybe add another example with an example and some other statement in an example group.

expect_no_offenses(<<~RUBY)
context 'when condition' do
it 'does something' do
end
it 'does something else' do
end
end
RUBY
end
end
Loading