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.

Sign up for GitHub

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

Jump to bottom

Fix false negative in RSpec/ExpectChange cop with block style and chained method call #1152

Merged
merged 2 commits into from May 25, 2021
Merged

Fix false negative in RSpec/ExpectChange cop with block style and chained method call #1152

merged 2 commits into from May 25, 2021

Conversation

tejasbubane
Copy link
Contributor

Closes #1143

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 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 in default/config.yml to the next minor version.

If you have modified an existing cop's configuration options:

  • Set VersionChanged in config/default.yml to the next major version.

@tejasbubane tejasbubane changed the title Fix false negative for RSpec/ExpectChange cop for block style with chained method call Fix false negative in RSpec/ExpectChange cop with block style and chained method call May 19, 2021
@tejasbubane
Copy link
Contributor Author

Rubocop failures are unrelated and shall be fixed in #1154

pirj
pirj approved these changes May 20, 2021
View reviewed changes
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.

Perfect, thank you!

@tejasbubane tejasbubane force-pushed the fix-1143 branch 2 times, most recently from ce61875 to 6cac7e1 Compare May 21, 2021 12:19
@tejasbubane tejasbubane requested a review from pirj May 21, 2021 12:25
pirj
pirj approved these changes May 21, 2021
View reviewed changes
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. 

Conflicting files
CHANGELOG.md

Prevents from merging 🤷

@tejasbubane tejasbubane requested a review from pirj May 21, 2021 16:26
pirj
pirj approved these changes May 21, 2021
View reviewed changes
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.

Thanks!

@pirj pirj requested review from bquorning and Darhazer May 21, 2021 16:37
Darhazer
Darhazer approved these changes May 24, 2021
View reviewed changes
lib/rubocop/cop/rspec/expect_change.rb Outdated
@@ -39,7 +39,7 @@ class ExpectChange < Base

# @!method expect_change_with_arguments(node)
def_node_matcher :expect_change_with_arguments, <<-PATTERN
(send nil? :change ({const send} nil? $_) (sym $_))
(send nil? :change $({const send} ...) (sym $_))
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to limit the argument of change at all?
Could be just (send nil? :change $_ (sym $_))

Copy link
Member

Choose a reason for hiding this comment

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

This way it would also match change(@ivar, :method), change($global, :method), change(::Const, :count) and other possible arguments.
I don't have a definitive list in my head, can there be any false positives? Something like change(-> {}, :call)?
👍 for such a change granted a few more examples are added.
@tejasbubane Are you up for that, or would you prefer us to file a ticket for the future?

Copy link
Member

Choose a reason for hiding this comment

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

Wondering if ${const send} can be a drop-in replacement for$({const send} ...) 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed as per @Darhazer's suggestion.

Copy link
Member

Choose a reason for hiding this comment

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

@bquorning What do you think?

@pirj
Copy link
Member

pirj commented May 25, 2021

@tejasbubane Wondering if you have the "allow maintainers to make changes" checkbox checked?
I tried pushing to the branch, and to your remote to no avail.

[fix-1143] $ git push
<creates new branch called `fix-1143`>

$ git remote add tejasbubane git@github.com:tejasbubane/rubocop-rspec.git
$ git push tejasbubane fix-1143
To github.com:tejasbubane/rubocop-rspec.git
 ! [remote rejected]   fix-1143 -> fix-1143 (permission denied)

🤷

Added a couple of specs to cover cases if @ivar $gvar ::CBase etc.

@tejasbubane
Copy link
Contributor Author

@pirj I have enabled it now

@pirj

This comment has been minimized.

Sign in to view
@pirj pirj merged commit 0be6148 into rubocop:master May 25, 2021
@pirj
Copy link
Member

pirj commented May 25, 2021

Thank you!

@Darhazer
Copy link
Member

Thank you for expanding the test suite 🙇

@tejasbubane tejasbubane deleted the fix-1143 branch May 28, 2021 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pirj pirj pirj approved these changes

@Darhazer Darhazer Darhazer approved these changes

@bquorning bquorning Awaiting requested review from bquorning

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

False negative in RSpec/ExpectChange with block style
3 participants
@tejasbubane @pirj @Darhazer