-
-
Notifications
You must be signed in to change notification settings - Fork 277
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
Update RSpec/IteratedExpectation
cop documentation with an example of a matcher relying on block arguments
#1659
base: master
Are you sure you want to change the base?
Conversation
No problem. I’ll do so tomorrow morning when I’m back at my laptop.
On June 7, 2023, zhangcaiyan ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In CHANGELOG.md
<#1659 (comment)>:
@@ -2,6 +2,7 @@
## Master (Unreleased)
+- Update `RSpec/IteratedExpectation` cop documentation with an
example of a matcher relying on block arguments. ***@***.***)
No change log entry is required because the change is a document
improvement and have no user impact. Can you remove this?
—
Reply to this email directly, view it on GitHub
<#1659 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAZ2GGRKQM5ER7TI75L6A4LXKE4MHANCNFSM6AAAAAAY6UWMNI>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
13f81c6
to
de6435d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even though it’s a way to appease the cop, I wouldn’t say it’s readable.
I would suggest extracting the block part i to a custom matcher, or a helper method.
Personally, I’m opposed at documenting this as it is now to prevent this style from being widely used.
@jprince ping :-) |
Apologies, I didn't realize you were waiting for my response. I don't disagree that the solution I documented is a bit difficult to read. However the current state is that the cop fails and the documentation provides no direction for how you can fix it - custom matcher or otherwise. IMO it would actually be ideal if the cop ignored this scenario (admittedly I'm not sure how challenging it would be, technically). I think most of us would agree that the alternative (what I've documented here) is worse than |
|
||
# good | ||
it 'sets inferred fields for users' do | ||
expect([user1, user2, user3]).to all(satisfy do |user| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would the following work?
‘’’
expect([user1, user2, user3]).to all have_attributes(
‘’’
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would certainly be more readable, but unfortunately not in this scenario (where block arguments are used). The challenge is that the have_attributes
call is receiving the user
as an argument. It would raise "undefined local variable or method `user'"
I agree that this is not a good use case for the all matcher and the original code might be a better fit. If @rubocop/rubocop-rspec agree with that direction, I'd be happy to make the change in the cop. If we do not change the cop however, I agree it would be good to have the documentation. Even if a custom matcher might be a more elegant solution, we have to point to at least some way to write the spec. |
This tripped me up but I was able to find a solution via #1206. Updating docs accordingly.
Before submitting the PR make sure the following are checked:
master
(if not - rebase it).CHANGELOG.md
if the new code introduces user-observable changes.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:
config/default.yml
.Enabled: pending
inconfig/default.yml
.Enabled: true
in.rubocop.yml
.VersionAdded: "<<next>>"
indefault/config.yml
.If you have modified an existing cop's configuration options:
VersionChanged: "<<next>>"
inconfig/default.yml
.