Conversation
manfred-brands
left a comment
manfred-brands
left a comment
There was a problem hiding this comment.
@Dreamescaper I like the idea and the implementation is relatively simple and even makes the ThatAsync simpler.
I had one question/suggestion to make this non-breaking.
| /// <summary> | ||
| /// Applies the constraint to a delegate that returns the task. | ||
| /// The default implementation simply evaluates the delegate and awaits the task | ||
| /// but derived classes may override it to provide for delayed processing. | ||
| /// </summary> | ||
| /// <typeparam name="TActual"></typeparam> | ||
| /// <param name="delTask"></param> | ||
| /// <returns></returns> | ||
| Task<ConstraintResult> ApplyToAsync<TActual>(Func<Task<TActual>> delTask); | ||
|
|
There was a problem hiding this comment.
Could we add this to an IAsyncConstraint interface?
When resolving the constraint in ThatAsync you can then test on IAsyncConstraint?
If not, throw a NotSupportedException?
That way it would be a non-breaking change.
There was a problem hiding this comment.
That makes sense.
I've made IAsyncConstraint internal for now - in case we'd need to more task-related methods later.
manfred-brands
left a comment
manfred-brands
left a comment
There was a problem hiding this comment.
Suggestion to replace BlockingDelay in async DelayedConstraint with Task.Delay
| } | ||
|
|
||
| [Test] | ||
| [Test, Platform(Exclude = "MACOSX", Reason = "Doesn't seem to work correctly with timing, something to ponder later")] |
There was a problem hiding this comment.
Drop this as a re-run of the build caused MacOs to pass. The machine github uses for MacOs seems to be very slow.
There was a problem hiding this comment.
I can see that other tests have this exclude as well, so I assume that it does happen from time time. Still, removed.
There was a problem hiding this comment.
...and it failed again :)
| while ((now = Stopwatch.GetTimestamp()) < delayEnd) | ||
| { | ||
| if (nextPoll > now) | ||
| ThreadUtility.BlockingDelay((int)TimestampDiff(delayEnd < nextPoll ? delayEnd : nextPoll, now).TotalMilliseconds); |
There was a problem hiding this comment.
Should we not use non-blocking' delay in an async method?
| ThreadUtility.BlockingDelay((int)TimestampDiff(delayEnd < nextPoll ? delayEnd : nextPoll, now).TotalMilliseconds); | |
| await Task.Delay(TimestampDiff(delayEnd < nextPoll ? delayEnd : nextPoll, now)); |
| } | ||
| } | ||
| if ((now = Stopwatch.GetTimestamp()) < delayEnd) | ||
| ThreadUtility.BlockingDelay((int)TimestampDiff(delayEnd, now).TotalMilliseconds); |
There was a problem hiding this comment.
Same here:
| ThreadUtility.BlockingDelay((int)TimestampDiff(delayEnd, now).TotalMilliseconds); | |
| await Task.Delay(TimestampDiff(delayEnd, now)); |
manfred-brands
left a comment
manfred-brands
left a comment
There was a problem hiding this comment.
Thanks @Dreamescaper Looks good to me now.
stevenaw
left a comment
stevenaw
left a comment
There was a problem hiding this comment.
LGTM too. Thanks again for another contribution @Dreamescaper !
|
Just noticed:
Is this so? |
|
@OsirisTerje |
|
@OsirisTerje |
4 participants
Fixes #4811 .
This PR is created for futher discussions.
Instead of ThatAsync awaiting the result and handling the exception, it delegates it to constraints - via ApplyAsync method.
Therefore, DelayedConstraint supports async tasks as well.
Obviusly, this is a breaking change. It will break anyone who implements IConstraint directly (instead of inheriting from Constraint).
Workarounds are possible to avoid that breaking change, but I'm not sure how ofter this interface is implemented direclty.