-
-
Notifications
You must be signed in to change notification settings - Fork 156
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
Reduce duplication in & between Mock & ObjectMethods #431
Open
nitishr
wants to merge
34
commits into
freerange:main
Choose a base branch
from
nitishr:mock-argument-iterator
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 29 commits
Commits
Show all changes
34 commits
Select commit
Hold shift + click to select a range
22c594f
make arg iteration similar in expects & stubs
nitishr ffac2ad
Refactor: extract add_expectation to DRY expects & stubs
nitishr f700c36
Refactor: inline temp - expectation
nitishr da280b1
Refactor: inline temp - iterator
nitishr 7579416
Refactor: extract anticipates to DRY expects,stubs
nitishr 7cff8fa
Refactor: inline temp - method
nitishr 7494b2d
Refactor: extract stub_method to be moved to Mockery
nitishr 5f83af5
Refactor: extract & reuse stubbed_method
nitishr b9422ba
Refactor: move stub_method to Mockery from ObjectMethods
nitishr 7a691d9
Refactor: make on_stubbing{,_method_unnecessarily} private
nitishr bf32420
Refactor: rename on_stubbingx to check_stubbingx
nitishr 73cf959
Refactor: no need for stateful obj for arg iter-n
nitishr b34726e
substitute algorithm for ArgumentIterator.each
nitishr 547a4d5
Refactor: extract anticipates to DRY expects,stubs
nitishr bf7ef4e
Refactor: inline add_expectation
nitishr 84e3ee7
no need to pass caller around
nitishr bfd20d8
ArgumentIterator.each returns the last result anyway
nitishr 9761be3
Refactor: inline temp - mocker to restrict scope
nitishr b2354df
call Mock#{expects,stubs} with methods_vs_return_values
nitishr 7e7a61b
Refactor: prep to move by inlining temp - method
nitishr b87733f
Refactor: replace ending yield w/ call in sequence
nitishr 2c4a5db
call stub_method alongside expectation setting for each method
nitishr 262ea4c
add expectation to expectations after fully set up
nitishr 5a5ed50
Refactor: rename stubbed_method->stubba_method_for
nitishr 5f1c5f4
Refactor: move ArgumentIterator#each to Mock
nitishr 365884e
Refactor: inline each_argument
nitishr 19004e7
Refactor: DRY up {PRE_RUBY_V19,RUBY_V19_AND_LATER}_EXCLUDED_METHODS
nitishr 201cfaa
Refactor: extract anticipates to DRY expects,stubs
nitishr 2840411
Refactor: inline method error_if_frozen
nitishr 583103b
more consistent setting of multiple expectations
nitishr b80cb98
move Mockery#stub_method call to ObjectMethods
nitishr 8a7af66
remove the Spanish Inquisition special case
nitishr 9930e5a
Refactor: inline anticipates into expects
nitishr 1906a10
Refactor: ExpectationSetting->CompositeExpectation
nitishr File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Although I appreciate that this reduces the duplication, I'm not convinced that the resultant code is any easier to follow. In fact I think, if anything, it's a bit harder to follow! Let me have a think about how we might better remove the duplication.
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.
Thanks for your review, @floehopper. Would you mind elaborating what aspects of this change you aren't convinced about and/or which ones you think make it harder to follow? If I understand your concerns more specifically, we might be able to come up with a better solution together.
To start with, I can share a bit more of my thinking here...
If it's the
anticipates
abstraction, I'll admit that I was initially unsure of it as well (and still am, but a lot less than I initially was). The reason I was unsure was I wondered if the need for me to look up (and cite the dictionary definitions in the PR description) was a sign of the concept being too smart/cute/nuanced.So, I let it mull for a few days in my head and reached the conclusion that
anticipates
is probably a better abstraction covering both an expectation and an allowance (stub, in mocha's terminology). I've always found expectation to be too strong a term to represent an allowance. (Anticipation might be too strong a term, too - but it does seem less strong than expectation.) So, in the long term, I would even consider/suggest considering renaming, for instance, the classExpectation
toAnticipation
(and maybe introduce subclasses -Expectation
andAllowance
). I have of course not fully analyzed that chain of reasoning, and certainly not considered it from an API compatibility point of view. This was only trying to clarify the concepts and their nuances for myself.Having said that, I'm not a native English speaker and may well have misinterpreted the shades of meanings of the various terms. So, I'd appreciate any corrections to my understanding or thinking.
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.
Sorry for the slow reply. I'll try to explain my reservations:
Although the
Mock#anticipates
method has removed all the duplication betweenObjectMethods#expects
,ObjectMethods#stubs
,Mock#expects
&Mock#stubs
(which is great BTW!), it's now really quite a long and "dense" method and, what's more, three of the lines are only executed conditionally. Coming at the final version of this method definition cold, I found it quite hard to follow.I'm not completely convinced that the inlining of the
ArgumentIterator
is a net gain. Encapsulating the common processing of the arguments for the four API methods in a single place still makes sense to me, even though its exact abstraction may not be quite right. And having to work out what the combination of calls toKernel#Array
,Array#map
,Array#flatten
,Array#shift
&Array#last
is doing adds quite a lot of cognitive overhead to understanding the method definition.The naming of
Mock#anticipates
doesn't bother me too much, although I don't think it is quite right. I wonder whether it would make more sense if we made anExpectation
have a cardinality ofCardinality.at_least(0)
(i.e. stubbing behaviour) by default, inlineMock#anticipates
intoMock#stubs
, and callMock#stubs
fromMock#expects
passing a block setting the expectation cardinality toCardinality.exactly(1)
(i.e. default expecting behaviour). It feels as if the expecting behaviour is a superset of the stubbing behaviour. What do you think...?Does make sense? I'm sorry I've struggled a bit to explain my thinking!
I've had time to go through the commits one-by-one in more detail this afternoon and I'd like to mention that I found them very easy to follow - so thank you for taking so much care to curate the commits and write useful commit messages.
Having looked through the commits in more detail, I'm inclined to have a go at merging a version of this PR, albeit with some changes as per my comments above. Is that OK with you?
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's very helpful, @floehopper. I now understand your concerns much better.
I like the suggestion of
expects
constraining theExpectation
, rather thanstubs
loosening it. Part of the reason I extractedanticipates
rather than reuseexpects
, BTW, was to avoid changing the public API of the existing methods, even though the changes would be fully backward compatible (i.e. accepting but not expecting a block or optional arguments). So, that's a consideration.I'd be happy with you merging a modified version of the PR with the changes you feel comfortable with. We could iterate on top of that for the more controversial/difficult aspects.
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.
👍
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.
@floehopper, I'm planning to push another commit which I think will alleviate some of your concerns, while at the same time, improving mocha's behavior with expectation chaining by making it more consistent across single-method and multiple-method stubs/expects.
The code in that commit or even the approach could possibly be improved, but I'd like to convey the general idea/direction before I go too far.
Is that OK? If you'd rather that I wait for you to finish merging (a version of) this PR, that's fine, too. Just let me know.
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.
Good point! 😄
Yes, that's fine - please go ahead!
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.
Done.
I'm completely fine taking any or all of the newly added commits to a different PR so they can be discussed and reviewed on their own (or altogether dropping any/all of them). The newly introduced
ExpectationSetting
may need some redesign, but I hope the changes convey the general concept and approach.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.
Looking at the code after my last few updates, I think the current form expresses the intent better than the change we were thinking of.
seem more intention-revealing and logical than
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.
Sorry for the slow reply. Thanks for adding the extra commit - I'm going to take a look at it shortly.