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 new Throws overloads that allow arguments to be passed to it #1191

Merged
merged 3 commits into from Jul 29, 2021

Conversation

adam-knights
Copy link
Contributor

@adam-knights adam-knights commented Jul 26, 2021

This implements #1048 (In Throws, we discussed in #1190 that we would leave ThrowsAsync alone for now).

The existing method SetReturnComputedValueBehavior in MethodCall.cs is slightly refactored so that those functions the new SetThrowComputedExceptionBehavior needs are available, some bits were not needed. I tried to find a balance here between duplication and the new function having its slightly different path and checks, but am more than happy to tweak this as needed. The diff here isn't great, probably better viewed side by side.

Copy link
Member

@stakx stakx left a comment

This looks great! Good work. 👍

I realise that the XML documentation comments, parameter names, etc. are a bit of a mess and not very consistent across the various files, but I think you did well in following the preexisting code patterns.

I found a few details that don't look quite right yet... but nothing major. Once those are cleaned up, this should be good to go!

src/Moq/Behaviors/ThrowComputedException.cs Outdated Show resolved Hide resolved
src/Moq/Behaviors/ThrowComputedException.cs Outdated Show resolved Hide resolved
src/Moq/Language/Flow/SetupSequencePhrase.cs Outdated Show resolved Hide resolved
src/Moq/Language/Flow/SetupSequencePhrase.cs Outdated Show resolved Hide resolved
src/Moq/Language/ISetupSequentialAction.cs Outdated Show resolved Hide resolved
src/Moq/Language/Flow/SetupPhrase.cs Outdated Show resolved Hide resolved
src/Moq/Language/IThrows.tt Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/Moq/Language/Flow/SetupPhrase.cs Outdated Show resolved Hide resolved
src/Moq/Language/Flow/SetupSequencePhrase.cs Show resolved Hide resolved
@adam-knights
Copy link
Contributor Author

adam-knights commented Jul 28, 2021

Thanks for all the feedback, really valuable and fixed/improved things. I've left the changes as separate commits as its easier for you to see, but if you want them rebased into the others before merge then that's not a problem

@adam-knights adam-knights requested a review from stakx Jul 28, 2021
@adam-knights
Copy link
Contributor Author

adam-knights commented Jul 28, 2021

Also, there were quite alot of fixes needed here, so fully anticipate we might need another round of smaller fixes.

Copy link
Member

@stakx stakx left a comment

Thanks for making the changes, @adam-knights! I'm quite happy with the PR as it stands now, but I've found two more details that might deserve another look... if you can fix those, that would be great!

Your commits are quite nicely done already, I'll let you decide whether you prefer to merge the review commits into the earlier ones or not. Both is fine with me.

Let me know when you're ready either for me to look at the last two remaining points, or for going ahead with the merge.

src/Moq/Language/Flow/SetupSequencePhrase.cs Show resolved Hide resolved
tests/Moq.Tests/SequentialActionExtensionsFixture.cs Outdated Show resolved Hide resolved
@adam-knights adam-knights force-pushed the throws-use-params-overloads branch from 7b456b9 to 9cc906a Compare Jul 29, 2021
@adam-knights adam-knights requested a review from stakx Jul 29, 2021
@adam-knights
Copy link
Contributor Author

adam-knights commented Jul 29, 2021

I tidied up the commits back to a nice three and added back that test. See my thoughts on the ThrowComputedException

stakx
stakx approved these changes Jul 29, 2021
@stakx stakx merged commit 0103409 into moq:main Jul 29, 2021
1 check passed
@stakx
Copy link
Member

stakx commented Jul 29, 2021

All good, let's do this! 🚀

Thanks for your contribution.

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