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

Improve docs for Expectation#with when it takes a block argument #682

Closed
floehopper opened this issue Nov 13, 2024 · 0 comments
Closed

Improve docs for Expectation#with when it takes a block argument #682

floehopper opened this issue Nov 13, 2024 · 0 comments

Comments

@floehopper
Copy link
Member

The current docs don't mention the block argument in the method description, although there is an example of its use and it does appear in the list of arguments.

It might also be worth saying there are no guarantees that the matcher won't be called more than once per invocation...?

floehopper added a commit that referenced this issue Nov 13, 2024
In the previous commit, the changes meant that a block provided to
`Expectation#with` was being invoked three times instead of once. The
original behaviour was not explicitly documented and there is code in
the wild [1] that relies on `Expectation#with` only being called once
per invocation.

I'm not convinced that test code should be relying on this behaviour,
but from a performance point-of-view, it probably makes sense to avoid
calling the matching methods on `ExpectationList` multiple times
unnecessarily. This commit changes the code in `Mock#handle_method_call`
to avoid unnecessary calls to these methods.

I've created #682 to suggest improving the docs for `Expectation#with`
when it takes a block argument to address the ambiguity that has become
apparent.

[1]: #681 (comment)
floehopper added a commit that referenced this issue Nov 13, 2024
In the previous commit, the changes meant that a block provided to
`Expectation#with` was being invoked three times instead of once. The
original behaviour was not explicitly documented and there is code in
the wild [1] that relies on `Expectation#with` only being called once
per invocation.

I'm not convinced that test code should be relying on this behaviour,
but from a performance point-of-view, it probably makes sense to avoid
calling the matching methods on `ExpectationList` multiple times
unnecessarily. This commit changes the code in `Mock#handle_method_call`
to avoid unnecessary calls to these methods.

I've created #682 to suggest improving the docs for `Expectation#with`
when it takes a block argument to address the ambiguity that has become
apparent.

[1]: #681 (comment)
floehopper added a commit that referenced this issue Nov 24, 2024
In the previous commit, the changes meant that a block provided to
`Expectation#with` was being invoked three times instead of once. The
original behaviour was not explicitly documented and there is code in
the wild [1] that relies on `Expectation#with` only being called once
per invocation.

I'm not convinced that test code should be relying on this behaviour,
but from a performance point-of-view, it probably makes sense to avoid
calling the matching methods on `ExpectationList` multiple times
unnecessarily. This commit changes the code in `Mock#handle_method_call`
to avoid unnecessary calls to these methods.

I've created #682 to suggest improving the docs for `Expectation#with`
when it takes a block argument to address the ambiguity that has become
apparent.

[1]: #681 (comment)
floehopper added a commit that referenced this issue Dec 8, 2024
Although there was already an example for this and the `yield`,
`yieldparam` & `yieldreturn` YARD tags were specified, I think it's
helpful to add some explanatory text to the method description.

Closes #682.
floehopper added a commit that referenced this issue Dec 8, 2024
Addressed an aside in the description of #682. This originally came up
in this commit [1].

[1]: 73c4ea6
floehopper added a commit that referenced this issue Dec 8, 2024
Addressed an aside in the description of #682. This originally came up
in this commit [1].

[1]: 73c4ea6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant