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

Record ReturnValue on CompletedFakeObjectCall #1697

Conversation

thomaslevesque
Copy link
Member

@thomaslevesque thomaslevesque commented Nov 13, 2019

Fixes #1696

WIP because I want to try another approach Scratch that, my idea won't work...

@thomaslevesque thomaslevesque changed the title [WIP] Record ReturnValue on CompletedFakeObjectCall Record ReturnValue on CompletedFakeObjectCall Nov 13, 2019
Copy link
Member

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks generally good, @thomaslevesque. I felt bad I hadn't told you to squash up earlier, but fortunately I had more comments and questions. In addition to the ones inline, I wonder if it isn't worthwhile testing a faked delegate as well? The implementation is slightly different. On the other hand, I do expect it to work, so I leave it to your discretion.

Copy link
Member

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant to "request changes", at least for the unfixedup comment.

And I just thought of another thing ICompletedFakeObjectCall.ReturnValue has a slightly misleading comment, as I noted in the issue. Perhaps a good idea to fix up while you're here, if you'd be so kind, to indicate that it's the value that was returned, not the value set to be returned.

@thomaslevesque
Copy link
Member Author

I wonder if it isn't worthwhile testing a faked delegate as well?

It is!

@thomaslevesque
Copy link
Member Author

And I just thought of another thing ICompletedFakeObjectCall.ReturnValue has a slightly misleading comment, as I noted in the issue. Perhaps a good idea to fix up while you're here, if you'd be so kind, to indicate that it's the value that was returned, not the value set to be returned.

Will do

Copy link
Member

@blairconrad blairconrad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Looking good. Squash it up!

@thomaslevesque
Copy link
Member Author

Squash it up!

Done

@blairconrad blairconrad merged commit 822e991 into FakeItEasy:support/5.x Nov 14, 2019
@blairconrad
Copy link
Member

Thanks, @thomaslevesque!

@blairconrad blairconrad mentioned this pull request Nov 14, 2019
@thomaslevesque
Copy link
Member Author

Thanks for the review!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants