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 mock.Protected().Verify overloads with additional exactParameterMatch parameter #753

Merged
merged 3 commits into from
Feb 21, 2019

Conversation

Shereef
Copy link
Contributor

@Shereef Shereef commented Feb 20, 2019

Closes #752.

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.

Awesome! Thank you very much for taking care of this! 👍

One additional request, could you please update the CHANGELOG.md as well? I would suggest the following two changes:

  1. Under the UnreleasedAdded heading:

     #### Added
    
     * New method overload `mock.Protected().Setup("VoidMethod", exactParameterMatch, args)` overload having a `bool exactParameterMatch` parameter. Due to method overload resolution, it was easy to think this already existed resolution when in fact it did not, leading to failing tests. (@stakx, #751)
    +* <Brief mention of the added method overloads, doesn't need to be as verbose as the above entry> (@Shereef, #753)
  2. Rephrase the current entry under the UnreleasedChanged section so it includes your added overloads:

    -* Method overload resolution for `mock.Protected().Setup("VoidMethod", ...)` may change due to a new overload: If you set up a `void` method whose first argument is a `bool`, make sure that argument gets interpreted as part of `args`, not as `exactParameterMatch` (see also *Added* section below). (@stakx, #751)
    +* Method overload resolution for `mock.Protected().Setup("VoidMethod", ...)` and `mock.Protected().Verify(...)` may change due to new overloads: If the first argument is a `bool`, make sure it gets interpreted as part of `args`, not as `exactParameterMatch` (see also *Added* section below). (@stakx & @Shereef, #751, #753)

Thank you for your work!

src/Moq/Protected/IProtectedMock.cs Show resolved Hide resolved
src/Moq/Protected/ProtectedMock.cs Outdated Show resolved Hide resolved
src/Moq/Protected/ProtectedMock.cs Outdated Show resolved Hide resolved
tests/Moq.Tests/VerifyFixture.cs Show resolved Hide resolved
tests/Moq.Tests/VerifyFixture.cs Show resolved Hide resolved
@stakx stakx changed the title Fixed #752 and added test/fact for it Add mock.Protected().Verify overloads with additional exactParameterMatch parameter Feb 20, 2019
@stakx
Copy link
Contributor

stakx commented Feb 20, 2019

I hope you don't mind me having altered your PR's title and description.

  • Regarding the changed title: Because GitHub will put the PR title into the merge commit message by default, and because the Moq source code might not live on GitHub for all eternity (at least in theory), it is good practice to make the PR titles independent of GitHub PR numbers and make them mostly self-explanatory.

  • Regarding the added description: GitHub treats a few special words as instructions; notice the dotted underline. Putting the words "fixes ...", "closes ..." or "resolves ..." into the description means that when the PR gets merged, the referenced issue gets automatically closed.;-)

@stakx stakx added this to the 4.11.0 milestone Feb 20, 2019
@Shereef
Copy link
Contributor Author

Shereef commented Feb 20, 2019

I wanted to know if there will be a Nuget build that has this soon, obviously, I would prefer it to be out sooner rather than later, however, I understand you may be holding off until v5 which is fine.

I have a workaround in place and wanted to know when should I plan on undoing this workaround. No rush. take you time.

EDIT: I found https://github.com/moq/moq4/milestone/8 so it just needs a few more issues no need to answer the above.

Copy link
Contributor Author

@Shereef Shereef left a comment

Choose a reason for hiding this comment

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

@stakx: I can't get it to change the status from changes requested, Am I doing something wrong ?

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 for the changes you made. 👍 If I may, I'd like to ask you to rephrase the changelog entries a little... then we'll be good to merge! See the review comments below for details.

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@stakx
Copy link
Contributor

stakx commented Feb 20, 2019

I can't get it to change the status from changes requested, Am I doing something wrong ?

Only the reviewer (and possibly other Moq team members) can change that status. So don't worry about it, you're doing the right thing. ;-)

(Btw. you can usually also leave the "resolve conversation" thingy to the reviewer who initiallycommented, or requested a change. It's a useful tool for them to track remaining changes.)

I wanted to know if there will be a Nuget build that has this soon, obviously, I would prefer it to be out sooner rather than later

You already found the milestone. To put its remaining issues into context of a concrete time frame, know that one of them is a rather large internal re-architecting that I have been planning and preparing for more than a year... so the 4.11.0 release is likely to take another two to three months, I'm afraid.

If there's more delay than that, I'll consider doing an ad-hoc 4.10.2 release including the non-breaking changes from 4.11.0.

@Shereef
Copy link
Contributor Author

Shereef commented Feb 21, 2019

You already found the milestone. To put its remaining issues into context of a concrete time frame, know that one of them is a rather large internal re-architecting that I have been planning and preparing for more than a year... so the 4.11.0 release is likely to take another two to three months, I'm afraid.

If there's more delay than that, I'll consider doing an ad-hoc 4.10.2 release including the non-breaking changes from 4.11.0.

Thanks for your response, there is no hurry, take your time, this can wait a few months!

(Btw. you can usually also leave the "resolve conversation" thingy to the reviewer who initiallycommented, or requested a change. It's a useful tool for them to track remaining changes.)

Will do!

you're doing the right thing. ;-)

Thanks for the assurance and the explanations!

@Shereef
Copy link
Contributor Author

Shereef commented Feb 21, 2019

I prefer it if we merge #755 first then I can fix the merge conflict and then this one so that the imports are looking good.

I am open to suggestions if you don't like #755 or want something else done.

I just want the parts I touch in the Test Fixture for Verify to be 100% up to par.

@stakx
Copy link
Contributor

stakx commented Feb 21, 2019

I've noticed that you've rewritten your PR to a few focused commits... awesome work, I couldn't ask for more. 😍

I prefer it if we merge #755 first then I can fix the merge conflict and then this one so that the imports are looking good.

I've merged #755. (Thanks again for that, btw.!)

Everything is looking good with this PR, but if you want to make any final changes (e.g. rebase it on top of master), you may do so now. Otherwise I'll go ahead with the merge in half a day or so.

@Shereef
Copy link
Contributor Author

Shereef commented Feb 21, 2019

I've noticed that you've rewritten your PR to a few focused commits... awesome work, I couldn't ask for more. 😍

I found a secret PR about contribution ^_^, my pleasure to comply!

I've merged #755. (Thanks again for that, btw.!)

I have rebased, fixed the first commit to comply with the grouping of imports then force pushed, it's really my pleasure! ;)

@stakx stakx merged commit 2d5810e into devlooped:master Feb 21, 2019
@stakx
Copy link
Contributor

stakx commented Feb 21, 2019

All good! 🚀 Your contribution is very much appreciated, thank you!

@Shereef Shereef deleted the 752-Fix branch February 21, 2019 19:14
@Shereef
Copy link
Contributor Author

Shereef commented Feb 21, 2019

All good! 🚀 Your contribution is very much appreciated, thank you!

Your repository and all the work you do for it is much more appreciated, Thank you!

@stakx
Copy link
Contributor

stakx commented Feb 21, 2019

Well, I'm actually just a maintainer, but not the owner of this project & repository... but thanks for the kinds words nevertheless. ❤️

@Shereef
Copy link
Contributor Author

Shereef commented Feb 21, 2019

You are a maintainer that replies and works on 24/7 to/on this repository you may not really own it, but it is clear to me that you are someone who takes ownership of whatever project they are handling and treats it accordingly, so thank you!

Let me know if there is anything I can do to make this ship sooner to the public or just generally help.

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