Add explanation for ExpectChange unsafe autocorrection #2061
Conversation
lovro-bikic
force-pushed
the
provide-safety-reason-for-expect-change
branch
2 times, most recently
from
972aed1 to
e8d554b
Compare
lovro-bikic
force-pushed
the
provide-safety-reason-for-expect-change
branch
from
e8d554b to
465d0c4
Compare
| # @safety | ||
| # Autocorrection is unsafe because `block` style evaluates the object | ||
| # twice (before and after evaluating `expect` block), whereas | ||
| # `method_call` style evaluates the object only once. |
There was a problem hiding this comment.
What do you mean by “object” here? Statement or a series of statements inside the block?
There was a problem hiding this comment.
After looking at the original PR, it’s clear to me. Nice catch, and thanks for marking the cop, it’s totally justified.
Can you please provide a bit more context in safety docs to make it obvious for the reader?
There was a problem hiding this comment.
If I understand #1174 correctly, the issue is that the method_call style calls message twice on the same receiver, so to speak. But with the block style, the entire block is evaluated twice – including instantiating two different receiver objects.
In #1174, the #time method is being called twice in each example, which confused me at first.
There was a problem hiding this comment.
What do you mean by “object” here? Statement or a series of statements inside the block?
I followed the terminology used in the previous section ("Enforces either passing object and attribute as...") which I think was in turn borrowed from the example in RSpec docs:
expect { do_something }.to change(object, :attribute)Instead of object/attribute, more precise terms would be receiver/message, but I'm not sure if that would be better understood in these docs?
There was a problem hiding this comment.
Interesting that rspec.info uses object/attribute, while the implementation uses receiver/message.
There was a problem hiding this comment.
I gave these docs a redo, and also added an example to Safety section to hopefully make it clearer. Can you have another look?
lovro-bikic
force-pushed
the
provide-safety-reason-for-expect-change
branch
from
465d0c4 to
da995f3
Compare
lovro-bikic
force-pushed
the
provide-safety-reason-for-expect-change
branch
from
da995f3 to
1d47c3d
Compare
#1178 marked
RSpec/ExpectChange's autocorrection as unsafe, but it didn't update the docs. This PR adds an explanation for the cop's unsafe autocorrection.Before submitting the PR make sure the following are checked:
master(if not - rebase it).CHANGELOG.mdif 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).