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

Conversation

tejasbubane
Copy link
Contributor

To check context with single example.

Closes #2001


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: "<<next>>" in default/config.yml.

@tejasbubane tejasbubane requested a review from a team as a code owner January 8, 2025 12:10
To check context with single example.

Closes rubocop#2001
@tejasbubane tejasbubane force-pushed the redundant-context-cop branch from b5295a5 to 4d58956 Compare January 8, 2025 12:26
@tejasbubane
Copy link
Contributor Author

tejasbubane commented Jan 8, 2025

Seems like we have a bunch of occurrences in this repo where we use single example inside context.

@koic
Copy link
Member

koic commented Jan 8, 2025

It’s doubtful whether this should be regarded as "redundant" or as an appropriate use of vocabulary, as it depend on individual judgment. The justification provided in #2001 doesn’t seem entirely sufficient, at the least, it seems this cop should be disabled by default.

@ydah
Copy link
Member

ydah commented Jan 8, 2025

IMO, I am reluctant to add this cop. Our project goal is to enforce the guidelines and best practices outlined in the community RSpec style guide. Currently, I am skeptical about whether this approach qualifies as a best practice. As a first step, it might be better to start by discussing in the RSpec Style Guide.

@bquorning
Copy link
Collaborator

I like this cop. I have often manually changed specs like

context "when foo" do
  it "does bar" do
  end
end

into

it "does bar when foo" do
end

but I agree that it should probably be disabled by default.

# @!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.

# @!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.

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 #rspec? :context _)
_
(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 _


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?

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.

@@ -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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New cop to discourage context with only a single test
5 participants