-
Notifications
You must be signed in to change notification settings - Fork 741
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
Fix inconsistent signature of GetOptions
method
#4647
Conversation
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=457326&view=codecoverage-tab |
@@ -59,7 +59,7 @@ internal ResilienceHandlerContext(AddResiliencePipelineContext<HttpKey> context) | |||
/// <remarks> | |||
/// If <paramref name="name"/> is <see langword="null"/> then the global options are returned. | |||
/// </remarks> | |||
public TOptions GetOptions<TOptions>(string name) => _context.GetOptions<TOptions>(name); | |||
public TOptions GetOptions<TOptions>(string? name = null) => _context.GetOptions<TOptions>(name); |
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.
Can we also add to the triple slash docs remarks section that if nothing gets passed then null will be used by default?
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.
Isn't that implied by default null value?
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.
For people looking at the code, yes, but people looking at docs.microsoft.com where this info is surfaced that won't be the case. You can either add it on the remarks, or on the param section instead.
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.
Done.
🎉 Good job! The coverage increased 🎉
Full code coverage report: https://dev.azure.com/dnceng-public/public/_build/results?buildId=458490&view=codecoverage-tab |
Noticed this while working on samples. The new signature matches the API of Polly, has null as a default.
Microsoft Reviewers: Open in CodeFlow