Add ValueTask extensions

[AdamDotNet]

Addresses #437

• Reference NuGet System.Threading.Tasks.Extensions
• Add extension methods to ReturnsExtensions.cs/.tt
• Add unit tests to ReturnsExtensionsFixture.cs

Tests pass on NetCoreApp2.0 and Net46 TFMs.

[stakx]

Thanks for the PR, this is looking good! Just a small number of detail changes (see below).

Also, could you please add an entry in the CHANGELOG.md (under the Unreleased > Added heading. Add a bullet at the bottom with a very brief summary of your change, along with your GitHub username and PR number in parentheses).

Thank you!

[stakx]

Please fix indentation.

[stakx]

Shouldn't the same package reference be added for the net46 target in both test projects? I suppose the main test project right now compiles only because it references the Moq project, which in turns references System.Threading.Tasks.Extensions.dll. I think it would be more correct if the test projects (both, for consistency) referenced the NuGet package directly.

(The same reference isn't needed for the netcoreapp2.0 target, the SDK already includes it.)

[AdamDotNet]

The reason why it "just works" without the explicit reference is because of the new style project file format automatically resolves transitive dependencies. In VS, expand the dependencies node -> .NetFramework 4.6 -> Projects -> Moq and you'll see System.Threading.Tasks.Extensions is indeed referenced by the unit test projects.

I can go ahead and add those references to the csproj and vbproj unit test files since you asked though.

[stakx]

Hmmmm, OK, if it's officially documented to work that way perhaps it just takes some getting used to on my part. :-) Thanks for teaching me something new. Both is fine with me then, it's your call!

[AdamDotNet]

No problem! I think I'll prefer the transitive dependency resolution then. Seems to be the more modern approach. See also Dotnet Project System; it's a feature pulled from the project.json/xproj days of the early .Net Core SDK and CLI tools.

[stakx]

I realise that you copy-pasted these documentation comments from the existing Task<T> methods, so this is more of a comment than asking for a change:

1. This parameter's name should have been setup, not mock: The .Returns "verb" acts on a setup of some mock, not on the mock itself. Unfortunately, renaming this parameter everywhere is theoretically a breaking change (albeit one unlikely to affect anyone, since we're talking of the this parameter). Let's leave it as is, better to be consistent everywhere.

2. I wonder if anyone readily understands that complicated summary description. :-) I would personally prefer something simpler, e.g. "The setup whose return value is configured." Feel free to change the description to something sensible everywhere, if you're in the mood & can spare the time.

[AdamDotNet]

On item 2., you're still referring to <param name="mock"> right? I read the param description several times and I think I finally get it haha. But since it is an extension method and most of the time extension methods are used with extension method syntax instead of as static method calls where you pass in the ISetup so most people using these methods won't actually see the comment in the IDE. All the same though, your suggestion slightly tweaked "The setup whose return value is to be configured." seems to make sense to me. I am just a little anxious about not finding every instance as you asked (assuming these kind of comments exist in places other than just the Task/ValueTask extensions).

[stakx]

With everywhere I actually meant only "everywhere in that particular class". Sorry if that wasn't clear enough. Since it's really a detail that doesn't make a huge difference, like you correctly say, I went ahead & merged your PR without this. :)

[stakx]

All good, thanks!

Perhaps you noticed that I deleted my review comment about merging GeneratedReturnsExtensions into ReturnsExtensions. I really prefer it the way you've changed it, yet I realised too late this rename will cause a binary-level breaking change. If someone updates the Moq DLL without recompiling their code, they'll see problems.

Not sure whether this is acceptable to end users, so I might end up reverting this particular change before releasing v4.8. :-/ Any thoughts on that?

[AdamDotNet]

I'm still a little novice at GitHub, so I guess I stuck to the comment in the email instead of noticing it was gone here on the website. Sorry about that!

I think you're right. I just refreshed my knowledge on SemVer and if that's what you follow for moq, then you can only make a breaking change like that with a major revision. I think there are a lot of cases where the code is recompiled when pulling new nuggets, but MissingMethodException is a thing if they just slip in the dll, or even when assembly binding redirects are in place.

If you can squirrel away that portion of the commit for a possible future v5, and maintain binary compatibility on v4, that's probably the safest bet.

Again, sorry I didn't put a little more thought into it in the first place!

[stakx]

If you can squirrel away that portion of the commit for a possible future v5, and maintain binary compatibility on v4, that's probably the safest bet.

Agreed. Even though this repo doesn't follow semver (the major version is basically fixed at 4 due to the repo's name, and the patch version gets computed automatically by GitInfo, so the only thing we have any real control over is the minor version)—I'm trying to stay as close to semver as possible. I'll probably revert that class name change.

Again, sorry I didn't put a little more thought into it in the first place!

My bad: I shouldn't have suggested that particular change to begin with.

Thank you for contributing!