-
Notifications
You must be signed in to change notification settings - Fork 461
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
andReturnArgument #1001
andReturnArgument #1001
Conversation
Resolves #992 |
library/Mockery/Expectation.php
Outdated
if (isset($args[$index])) { | ||
return $args[$index]; | ||
} | ||
throw new Exception("Cannot return an argument value. No argument exists for the index $index"); |
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.
I'm not 100% sure if throwing an exception here is correct.
What do people think is the right behaviour when the argument index is out-of-bounds for a particular method call?
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.
I think it's a reasonable starting point. Could throw a OutOfBoundsException
to be more explicit.
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.
makes sense
library/Mockery/Expectation.php
Outdated
throw new \InvalidArgumentException("Invalid argument index supplied. Index must be a positive integer."); | ||
} | ||
$closure = function (...$args) use ($index) { | ||
if (isset($args[$index])) { |
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.
Probably want array_key_exists
here, the argument could legitimately be null
. Test case to cover would be good too.
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 catch, I'll update both
library/Mockery/Expectation.php
Outdated
* @param int $index | ||
* @return self | ||
*/ | ||
public function andReturnArgument($index) |
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.
I'm not 100% sure what would be best, but in other public methods, we use withArgs
, rather than withArguments
.
So would we better with andReturnArg
or is Arg
too ambiguous?
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.
I think andReturnArg
is fine; I'm happy to keep things consistent.
Sorry for the delay! Missed the notification on this one. |
No worries, thanks for the review |
@davedevelopment I've updated this MR according to your feedback |
Thank you, great stuff |
Added functionality to return an argument passed to a method expectation