-
-
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 new Throws overloads that allow arguments to be passed to it #1191
Add new Throws overloads that allow arguments to be passed to it #1191
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.
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!
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 |
Also, there were quite alot of fixes needed here, so fully anticipate we might need another round of smaller fixes. |
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 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.
I tidied up the commits back to a nice three and added back that test. See my thoughts on the |
All good, let's do this! 🚀 Thanks for your contribution. |
This implements #1048 (In
Throws
, we discussed in #1190 that we would leaveThrowsAsync
alone for now).The existing method
SetReturnComputedValueBehavior
inMethodCall.cs
is slightly refactored so that those functions the newSetThrowComputedExceptionBehavior
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.