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

Fix a false positive for RSpec/RepeatedSubjectCall #1822

Merged
merged 1 commit into from Mar 1, 2024
Merged

Fix a false positive for RSpec/RepeatedSubjectCall #1822

merged 1 commit into from Mar 1, 2024

Conversation

ydah
Copy link
Member

@ydah ydah commented Mar 1, 2024

Fix: #1821


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

@ydah ydah requested a review from a team as a code owner March 1, 2024 10:54
Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

A nice simplification and a quick fix. Thank you!

walker = subject_node
def detect_offense(subject_node)
subject_node.each_ancestor(:block) do |block|
next if subject_node.chained?
Copy link
Member

Choose a reason for hiding this comment

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

I have never seen this method. Is it old enough for us to be sure it will be there with a minimum required rubocop and rubocop-ast versions we require?

Copy link
Member

Choose a reason for hiding this comment

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

It was there like forever. I just wonder if we should not just skip the whole each_ancestor in this case, instead of skipping each iteration 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

we should not just skip the whole each_ancestor in this case, instead of skipping each iteration 😅

Indeed! Thank you catching this.

Copy link
Member

@Darhazer Darhazer left a comment

Choose a reason for hiding this comment

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

I have no objections against this change.

The original spec however is light on use cases. What happens for instance if you access subject in a hook, let's say configuring a stub like allow(Foo).to receive(:bar).and_return(subject) (which might also use block synax allow(Foo).to receive(:bar) { subject }. It's hard to tell from reading the code alone

@ydah
Copy link
Member Author

ydah commented Mar 1, 2024

The original spec however is light on use cases. What happens for instance if you access subject in a hook, let's say configuring a stub like allow(Foo).to receive(:bar).and_return(subject) (which might also use block synax allow(Foo).to receive(:bar) { subject }. It's hard to tell from reading the code alone

Indeed. I'll make a follow-up PR.

@ydah ydah merged commit 6b37836 into master Mar 1, 2024
24 checks passed
@ydah ydah deleted the fix1821 branch March 1, 2024 17:44
@ydah ydah restored the fix1821 branch March 1, 2024 17:44
@ydah ydah deleted the fix1821 branch March 1, 2024 17:44
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.

RSpec/RepeatedSubjectCall flags a false positive
4 participants