ContinueWith overloads #16

AlphaGremlin · 2018-04-21T08:17:27Z

Add ContinueWith overloads with the state parameter.

Found a few more that I'd missed in the previous PR, so I've added those.
Also, I've switched to handling the state value manually, because the compiler was creating 20 separate copies of exactly the same underlying class.

Add ContinueWith overloads with the state parameter

jnm2 · 2018-04-21T18:06:09Z

src/AsyncBridge/AsyncCompatLibExtensions.cs

+    /// <param name="task">The target Task</param>
+    /// <param name="action">The continuation method to execute</param>
+    /// <param name="state">A state object to pass to the continuation</param>
+    /// <returns>A task representing the continuation status</returns>


What do you think about copying the documentation from https://github.com/Microsoft/referencesource/blob/b31308b03e8bd5bf779fb80fda71f31eb959fe0b/mscorlib/system/threading/Tasks/Task.cs#L3932-L3948?

The idea of copying the documentation also brings to light that the BCL uses the parameter name continuationAction which we should follow for source compatibility if people use named parameters.

jnm2

I like it!
I would feel most comfortable with test coverage of every code path, boring as that is. 😬 Sorry to be a stick-in-the mud!

jnm2 · 2018-04-21T18:17:46Z

src/AsyncBridge/AsyncCompatLibExtensions.cs

+    private sealed class ContinueWithState
+    {
+        private readonly Action<Task, object> _Callback;
+        private readonly object _State;


I've never seen the _Upper naming convention before. I apologize that we don't have an .editorconfig set up.
The polyfill code, copying Microsoft, uses m_camelCase. My personal preference for private fields would be camelCase but if you don't like that, _camelCase would be fine.

jnm2 · 2018-04-21T18:20:21Z

src/AsyncBridge/AsyncCompatLibExtensions.cs

+    /// <param name="task">The target Task</param>
+    /// <param name="action">The continuation method to execute</param>
+    /// <param name="state">A state object to pass to the continuation</param>
+    /// <returns>A task representing the continuation status</returns>


The idea of copying the documentation also brings to light that the BCL uses the parameter name continuationAction which we should follow for source compatibility if people use named parameters.

jnm2 · 2018-04-21T18:22:40Z

src/AsyncBridge/AsyncCompatLibExtensions.cs

+    /// <returns>A task representing the continuation status</returns>
+    public static Task ContinueWith(this Task task, Action<Task, object> action, object state)
+    {
+        return task.ContinueWith(new ContinueWithState(action, state).ContinueWith);


Please match BCL behavior by throwing new ArgumentNullException(nameof(continuationAction)). Right now ContinueWithState.ContinueWith throws a NullReferenceException when the continuation executes rather than throwing an ArgumentNullException before even scheduling the continuation, as the BCL does.

(That's a reminder to me that we need tests verifying this behavior too.)

Added XML documentation from ReferenceSource Changed parameter names to be compatible

AlphaGremlin · 2018-04-22T02:08:39Z

So, apparently the AsyncTargetingPack does not include any ContinueWith extension methods. I'll have to disable the ContinueWith tests for that target.

Added ContinueWith overload tests

jnm2

One last thing I missed until I ran code coverage! I added integration with codecov.io, so if you rebase the PR on the latest updates to master, you'll get to see it here.

jnm2 · 2018-04-23T01:59:09Z

tests/AsyncBridge.Tests/ContinueWithTests.cs

+        public void Action()
+        {
+            var GotState = false;
+            TestUtils.RunAsync(() => TaskEx.Run(() => { }).ContinueWith((task, state) => GotState = (bool)state, true));


This isn't actually testing the Action<Task, object> overload. Because of the lambda's expression body, the compiler is inferring it as Func<Task, object, bool>.
You can see that by the red lines in the coverage report here: https://codecov.io/gh/OmerMor/AsyncBridge/src/2806bdff8e65cfb4be247c1595fc319cdc3baa19/src/AsyncBridge/AsyncCompatLibExtensions.cs#L779...797

(needs to be fixed in 5 places)

Damn it! I'll fix that up.

One thing I really miss from SharpDevelop is the code coverage integration. VS only offers it in the enterprise version.

jnm2 · 2018-04-23T02:04:02Z

tests/AsyncBridge.Tests/ContinueWithTests.cs

+        public void ActionResult()
+        {
+            var GotState = false;
+            TestUtils.RunAsync(() => TaskEx.Run(() => true).ContinueWith((task, state) => GotState = (bool)state, true));


Same deal, testing the Func<Task<bool>, object, bool> overload instead of the Action<Task<bool>, object> overload.

https://codecov.io/gh/OmerMor/AsyncBridge/src/2806bdff8e65cfb4be247c1595fc319cdc3baa19/src/AsyncBridge/AsyncCompatLibExtensions.cs#L799...817

(5 places)

Fixed tests using the wrong overloads

codecov-io · 2018-04-25T07:47:37Z

Codecov Report

Merging #16 into master will increase coverage by 4.61%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #16      +/-   ##
=========================================
+ Coverage   58.78%   63.4%   +4.61%     
=========================================
  Files          17      17              
  Lines         461     511      +50     
  Branches       60      64       +4     
=========================================
+ Hits          271     324      +53     
+ Misses        166     163       -3     
  Partials       24      24

Impacted Files	Coverage Δ
src/AsyncBridge/AsyncCompatLibExtensions.cs	`87.09% <100%> (+53.76%)`	⬆️
...cBridge/Runtime/TargetedPatchingOptOutAttribute.cs	`0% <0%> (ø)`	⬆️
src/AsyncBridge/Threading.Tasks/TaskEx.cs	`70.29% <0%> (+2.97%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0ab620...c7593cb. Read the comment docs.

AlphaGremlin · 2018-04-25T07:51:05Z

I think I did the rebase correctly...? It seems to have code coverage now. That should have fixed the tests.

jnm2 · 2018-04-25T17:48:21Z

@AlphaGremlin Awesome! All the code looks ready! A quick explanatory detour next, for which I apologize!

ℹ You did a merge, which is different than a rebase. A merge is when you join back together at the end with a merge commit that has two parent comments. A rebase is when you rewrite your commits to change your first commit's parent commit.

GitHub reviews just happen to not work very well when you merge the master branch into your topic branch (PR branch). It counts all these changes from master as part of your diff, so it's hard to tell what you wrote versus what you pulled in from master. There's also a superfluous merge commit at the end:

This explains why Codecov is still complaining that it can't find coverage for the master commit you're based off of, 6cb68ef. With a rebase, your first commit's parent would be d5ff32a instead (the current tip of master).

➡ Don't worry about rebasing in this PR. I can see that your changes have 100% coverage and I'd like to merge them.

⚠ I'd prefer to undo the merge with master first, though, so the diff in this PR is usable for anyone that looks back at it. Sorry about that! It's super easy though:

git reset c7593cb --hard
This makes your local continuewith branch point at c7593cb, “Fixed overload tests.” The merge commit no longer has something pointing at it, so it will be discarded. Now your history and diff is fixed!
git push -f
This makes your remote origin/continuewith branch point at the same commit as your local branch. -f (--force) is needed because we are causing the remote branch to rewind its history by dropping that merge commit.

You've been really great and put up with a lot through this whole process and I appreciate that very much! I can't wait to get this in. 👍

AlphaGremlin · 2018-04-26T00:29:24Z

Done :)

jnm2 · 2018-04-26T03:57:53Z

🎉 Your tireless work is much appreciated! Thank you!

AlphaGremlin added 2 commits April 21, 2018 18:12

Add more extension methods

322eb79

Add ContinueWith overloads with the state parameter

Missing overload state wrapper

1e9ad43

jnm2 added the enhancement label Apr 21, 2018

jnm2 reviewed Apr 21, 2018

View reviewed changes

jnm2 requested changes Apr 21, 2018

View reviewed changes

XMLDoc and Parameter Names

d74e759

Added XML documentation from ReferenceSource Changed parameter names to be compatible

Add ContinueWith tests

c57638e

Added ContinueWith overload tests

jnm2 requested changes Apr 23, 2018

View reviewed changes

Fixed overload tests

c7593cb

Fixed tests using the wrong overloads

jnm2 approved these changes Apr 26, 2018

View reviewed changes

jnm2 merged commit a49f0f0 into OmerMor:master Apr 26, 2018

jnm2 mentioned this pull request May 14, 2018

Integrate Code Coverage into CI nunit/nunit#423

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ContinueWith overloads #16

ContinueWith overloads #16

AlphaGremlin commented Apr 21, 2018

jnm2 Apr 21, 2018 •

edited

jnm2 Apr 21, 2018

jnm2 left a comment

jnm2 Apr 21, 2018

jnm2 Apr 21, 2018

jnm2 Apr 21, 2018 •

edited

AlphaGremlin commented Apr 22, 2018

jnm2 left a comment

jnm2 Apr 23, 2018

AlphaGremlin Apr 23, 2018

jnm2 Apr 23, 2018

codecov-io commented Apr 25, 2018 •

edited

AlphaGremlin commented Apr 25, 2018

jnm2 commented Apr 25, 2018 •

edited

AlphaGremlin commented Apr 26, 2018

jnm2 commented Apr 26, 2018

ContinueWith overloads #16

ContinueWith overloads #16

Conversation

AlphaGremlin commented Apr 21, 2018

jnm2 Apr 21, 2018 • edited

Choose a reason for hiding this comment

jnm2 Apr 21, 2018

Choose a reason for hiding this comment

jnm2 left a comment

Choose a reason for hiding this comment

jnm2 Apr 21, 2018

Choose a reason for hiding this comment

jnm2 Apr 21, 2018

Choose a reason for hiding this comment

jnm2 Apr 21, 2018 • edited

Choose a reason for hiding this comment

AlphaGremlin commented Apr 22, 2018

jnm2 left a comment

Choose a reason for hiding this comment

jnm2 Apr 23, 2018

Choose a reason for hiding this comment

AlphaGremlin Apr 23, 2018

Choose a reason for hiding this comment

jnm2 Apr 23, 2018

Choose a reason for hiding this comment

codecov-io commented Apr 25, 2018 • edited

Codecov Report

AlphaGremlin commented Apr 25, 2018

jnm2 commented Apr 25, 2018 • edited

AlphaGremlin commented Apr 26, 2018

jnm2 commented Apr 26, 2018

jnm2 Apr 21, 2018 •

edited

jnm2 Apr 21, 2018 •

edited

codecov-io commented Apr 25, 2018 •

edited

jnm2 commented Apr 25, 2018 •

edited