-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[Docs] Add safe execution section to the migration guide #1638
[Docs] Add safe execution section to the migration guide #1638
Conversation
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1638 +/- ##
=======================================
Coverage 84.63% 84.63%
=======================================
Files 306 306
Lines 6819 6819
Branches 1045 1045
=======================================
Hits 5771 5771
Misses 839 839
Partials 209 209
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
||
// Asynchronous execution | ||
var context = ResilienceContextPool.Shared.Get(); | ||
Outcome<int> pipelineResult = await pipeline.ExecuteOutcomeAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@martincostello , @peter-csala
Wondering whether it makes sense to add more ExecuteOutcome
convenience overloads so we do not force passing ResilienceContext
and state
?
Or maybe wait for feedback until someone really needs these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be easy enough for people to just use a discard if they don't need them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO it would definitely ease the usage of the ExecuteOutcomeAsync
method.
I think using context and state can be considered as more advance usage. For basic scenarios they are just there without utilizing them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One reason why there is only a single method is that I considered this advanced API that won't be used that much. But maybe I am wrong and it will be commonly used.
In that case having some convenience overloads makes sense (don't force people to use ResilienceContextPool.Shared.Get()
).
But maybe let's just have "wait and see" approach? Adding new APIs won't be a breaking change anyway.
|
||
// Asynchronous execution | ||
var context = ResilienceContextPool.Shared.Get(); | ||
Outcome<int> pipelineResult = await pipeline.ExecuteOutcomeAsync( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be easy enough for people to just use a discard if they don't need them?
Looks like this has picked up some merge conflicts. |
The issue or feature being addressed
Details on the issue fix or feature implementation
Confirm the following