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

Deprecate top_level_group? and test it in a Cop class #1976

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 CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

## Master (Unreleased)

- Deprecate `top_level_group?` method from `TopLevelGroup` mixin as all of its callers were intentionally removed from `Rubocop/RSpec`. ([@corsonknowles])

## 3.2.0 (2024-10-26)

- Fix `RSpec/VoidExpect` to only operate inside an example block. ([@corsonknowles])
Expand Down
7 changes: 7 additions & 0 deletions lib/rubocop/cop/rspec/mixin/top_level_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ module RSpec
module TopLevelGroup
extend RuboCop::NodePattern::Macros

DEPRECATED_MODULE_METHOD_WARNING =
'top_level_group? is deprecated and will be ' \
'removed in the next major version of rubocop_rspec.'

def on_new_investigation
super

Expand All @@ -28,7 +32,10 @@ def on_top_level_example_group(_node); end

def on_top_level_group(_node); end

# @private
# @deprecated All callers of this method have been removed.
def top_level_group?(node)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def top_level_group?(node)
# @private
# @deprecated <some description on how to replace>
def top_level_group?(node)

This is more a reminder for us not to forget to remove in 4.0
Should we file a ticket?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like an Issue? Or is there an internal ticketing system?

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 added the @deprecated declaration, but I'm still not sure what filing a ticket entails.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, we just use issues. I created a 4.0 milestone and opened #1986.

warn DEPRECATED_MODULE_METHOD_WARNING, uplevel: 1
top_level_groups.include?(node)
end

Copy link
Member

Choose a reason for hiding this comment

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

Something feels off about the node.children line.
I vaguely recall that a single-statement block and a multi-statement block’s AST are different (yuck!). Will the result be correct in both cases?
Like
foo { RSpec.describe { } } vs foo { RSpec.describe { }; RSpec.shared_examples { } }?

Unrelated to this PR, just an observation.
Might as well be a false alarm, and I apologise in advance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds like more specs are needed!

Expand Down
24 changes: 24 additions & 0 deletions spec/rubocop/cop/rspec/mixin/top_level_group_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# frozen_string_literal: true

RSpec.describe RuboCop::Cop::RSpec::TopLevelGroup do
describe '#top_level_group?' do
let(:stub_class) do
Class.new do
include RuboCop::Cop::RSpec::TopLevelGroup

def initialize
@top_level_groups = []
end

def test_top_level_group
top_level_group?(nil)
end
end
end

it 'warns because it is deprecated' do
expect { stub_class.new.test_top_level_group }.to \
output(/warning: top_level_group\? is deprecated/).to_stderr
end
end
end