Improve usability of ResilienceStrategy<T>#1428
Conversation
There was a problem hiding this comment.
Just not a big fan of the name T as it's not very descriptive alongside TResult and TState and might cause some confusion between T and TResult when both are kinda a result (just in different contexts).
| /// <exception cref="ArgumentNullException">Thrown when <paramref name="callback"/> or <paramref name="context"/> is <see langword="null"/>.</exception> | ||
| public ValueTask<TResult> ExecuteAsync<TState>( | ||
| Func<ResilienceContext, TState, ValueTask<TResult>> callback, | ||
| public ValueTask<T> ExecuteAsync<T, TState>( |
There was a problem hiding this comment.
I wonder if there's a better name we can give this rather than T?
There was a problem hiding this comment.
Maybe just swap T with TResult?
So it will be ResilienceStrategy<T> but individual methods will have TResult?
There was a problem hiding this comment.
I didn't have any good suggestions - your one sounds fine to me.
Codecov Report
@@ Coverage Diff @@
## main #1428 +/- ##
=======================================
Coverage 83.92% 83.92%
=======================================
Files 275 275
Lines 6526 6526
Branches 1007 1007
=======================================
Hits 5477 5477
Misses 840 840
Partials 209 209
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Details on the issue fix or feature implementation
These changes enable flowing the type used in the callback back to the result:
Previously, these all would be object:
Confirm the following