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

Add support for void sequences (2nd iteration) #463

Merged
merged 12 commits into from
Oct 2, 2017

Conversation

alexbestul
Copy link
Contributor

Supersedes PR #454
Resolves Issue #451

@alexbestul
Copy link
Contributor Author

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:

  • I only added the license text to new files that I created--not to any existing files that I modified. I'm not sure of the legal implications of me adding the license to existing code :-) .
  • I left all of my work-in-progress commits intact for now. Once you give a thumbs-up on the changes, I can squash them (or, of course, you can use GitHub's "Squash and Merge" option to the same effect).
  • I did try adding Async again. I got it to work without affecting any existing APIs, but my implementation seemed pretty kludgy. I think it's better to get the non-async API merged, and then revisit async another day.

Copy link
Contributor

@stakx stakx left a 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)
Copy link
Contributor

@stakx stakx Oct 2, 2017

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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: ExuecuteExecute

Copy link
Contributor

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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: ExuecuteExecute

/// Configures the next call in the sequence to throw an exception.
/// </summary>
/// <example>
/// The following code configures the first call to <c>Exuecute()</c>
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: ExuecuteExecute

@stakx
Copy link
Contributor

stakx commented Oct 2, 2017

I only added the license text to new files that I created--not to any existing files that I modified. I'm not sure of the legal implications of me adding the license to existing code :-) .

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.

I left all of my work-in-progress commits intact for now. Once you give a thumbs-up on the changes, I can squash them (or, of course, you can use GitHub's "Squash and Merge" option to the same effect).

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.

I did try adding Async again. I got it to work without affecting any existing APIs, but my implementation seemed pretty kludgy. I think it's better to get the non-async API merged, and then revisit async another day.

Agreed. You did some solid work here, let's get it merged and worry about the async bits later!

@stakx stakx merged commit 48c6aa6 into devlooped:develop Oct 2, 2017
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