-
-
Notifications
You must be signed in to change notification settings - Fork 794
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
Added support for lambda expressions while creating a mock #888
Added support for lambda expressions while creating a mock #888
Conversation
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.
Looks good overall! I'd like to give this another, closer look, unfortunately I'm running out of time today. In the meantime, could you please take care of the following few details?
Thank you!
@@ -5,7 +5,7 @@ | |||
using System.Collections.Generic; | |||
using System.ComponentModel; | |||
using System.Linq; | |||
|
|||
using System.Linq.Expressions; | |||
using Moq.Properties; |
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.
Please insert a blank line above this line, so that imports are grouped by the main namespace (System.*
, Moq.*
).
Looks all good to me. 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.
Sorry for the wait, @frblondin.
I've got a few more things, I've been paying some more attention to fitting this new feature into the existing codebase in a consistent manner... so there's some nitpicking involved. I hope you don't mind.
I integrated most your changes: except the new overloads to avoid use of optional parameters (creates ambiguities) and your question about use of lambda syntax for parameter-less constructors. Hope this version will be the good one ;-) |
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.
Nearly there. :-)
(Sorry for the nitpicking, it's easier to get these things done right now while we're focusing on it, rather than later.)
except the new overloads to avoid use of optional parameters (creates ambiguities)
Ah, right you are! Agreed, let's keep the optional parameters. 👍
New round of updates, I believe your remaining change requests have been taken into account - you to confirm & resolve! Thanks for you time. |
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.
Thank you!
There is another whitespace issue in Resources.resx
(a succession of redundant blank lines) but I've caused you as much work there as I dare 😆, so if you don't mind I'll clean this up myself quickly after merging.
🚀
Implements #884.
The following methods have been added:
As per your remark in the issue, I favored the use of an optional parameter for the behavior. Using an optional parameter doesn't allow us to keep the same order because the behavior is necessary the last parameter (the one being optional...).
The implementation works as follows:
NewExpression
is visited, an internal lambda will be created & invoked to produce the arguments as an instance ofobject[]
I was about to suggest to mark the existing mock creation method/constructors (taking
params object[]
) as deprecated... but in any case we still need them when creating a mock of an abstract class!I added unit tests & updated the changelog.