-
-
Notifications
You must be signed in to change notification settings - Fork 803
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
Add support for void sequences (2nd iteration) #463
Add support for void sequences (2nd iteration) #463
Conversation
This PR addresses the feedback in #454. I decided to open a new PR because the changes included renaming several files and rebasing the branch, which would have clobbered the history/comments on the existing PR. A few things worth mentioning:
|
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 a lot for the changes, this is looking good & mergeable. There are a few minor details, mostly typos (see line comments). If you want to adjust these & do the commit squashing yourself, feel free to do so; otherwise, I'll squash-merge this in about 10 hours.
CHANGELOG.md
Outdated
|
||
#### Added | ||
|
||
* Support for sequential setup of `void` methods (@alexbestul, #451) |
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.
#451
isn't strictly wrong, as it still gives people a link to the GitHub issues and discussions, but I guess #463
would be more accurate now. :-)
/// Configures the next call in the sequence to do nothing. | ||
/// </summary> | ||
/// <example> | ||
/// The following code configures the first call to <c>Exuecute()</c> |
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.
Typo: Exuecute
→ Execute
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.
Note that as there are other typos in Moq's code base, I'm OK with letting this stand, then do a focused typo hunt across the whole solution sometime in the future. (But of course you can fix it now if you want.)
/// Configures the next call in the sequence to throw an exception. | ||
/// </summary> | ||
/// <example> | ||
/// The following code configures the first call to <c>Exuecute()</c> |
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.
Typo: Exuecute
→ Execute
/// Configures the next call in the sequence to throw an exception. | ||
/// </summary> | ||
/// <example> | ||
/// The following code configures the first call to <c>Exuecute()</c> |
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.
Typo: Exuecute
→ Execute
I think you did the right thing. After all, the purpose of your PR is adding a new feature, not fixing other pre-existing stuff. ;) Adding the license text to existing files wouldn't have hurt, I believe, but it's probably more efficient to do a separate and focused "add missing license texts" round across the whole code base.
See review comment above. Both is fine with me, I'll give you a few hours if you want to do this, but I'm happy to merge as is or squash-merge.
Agreed. You did some solid work here, let's get it merged and worry about the async bits later! |
Supersedes PR #454
Resolves Issue #451