feat(extensions): add EnableReloads overload accepting custom IOptionsMonitor#3140
Open
arashzjahangiri wants to merge 2 commits into
Open
Conversation
…sMonitor Closes App-vNext#3050 Adds a new public overload of AddResiliencePipelineContext<TKey>.EnableReloads<TOptions> that accepts a caller-supplied IOptionsMonitor<TOptions> directly, instead of resolving it from the DI container. The underlying ConfigureBuilderContextExtensions.EnableReloads already accepts the monitor directly - this change surfaces that path through the public API. Use cases enabled: - Custom options systems outside Microsoft.Extensions.Options - Wrapping alternative configuration sources (feature flags, remote configs) - Sharing monitor instances across pipelines without DI registration
| using Polly.Registry; | ||
| using Polly.Utils; | ||
|
|
||
| #pragma warning disable RS0026 // Do not add multiple public overloads with optional parameters |
Member
There was a problem hiding this comment.
Please rework the API to remove the need to suppress this warning.
Author
There was a problem hiding this comment.
Reworked the API to avoid the RS0026 suppression entirely. The new IOptionsMonitor overload no longer has an optional name parameter — instead there are two explicit overloads: one with just the monitor, and one with both monitor and name. The original EnableReloads<TOptions>(string? name = null) remains unchanged (no breaking change). The RS0027 that appears on the original overload (a consequence of adding a 2-param sibling) is suppressed with a targeted pragma/restore pair on that method only, consistent with the pattern already used in Polly.Core.
| #nullable enable | ||
| abstract Polly.Telemetry.MeteringEnricher.Enrich<TResult, TArgs>(in Polly.Telemetry.EnrichmentContext<TResult, TArgs> context) -> void | ||
| Polly.DependencyInjection.AddResiliencePipelineContext<TKey> | ||
| Polly.DependencyInjection.AddResiliencePipelineContext<TKey>.EnableReloads<TOptions>(string? name = null) -> void |
Member
There was a problem hiding this comment.
This is a breaking change.
…suppressions Replace the file-level RS0026 suppression with an API design that needs no suppression: - Keep the shipped EnableReloads<TOptions>(string? name = null) overload unchanged - Add EnableReloads<TOptions>(IOptionsMonitor<TOptions> monitor) (no-name convenience) - Add EnableReloads<TOptions>(IOptionsMonitor<TOptions> monitor, string? name) without optional parameter, eliminating the RS0026 violation - Add a targeted RS0027 suppress/restore around the original overload only, consistent with the pattern used in Polly.Core for the same constraint
arashzjahangiri
force-pushed
the
fix/enable-reloads-custom-options-monitor
branch
from
c52d8f8 to
a260473
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #3050
Adds a new public overload of AddResiliencePipelineContext.EnableReloads that accepts a caller-supplied IOptionsMonitor directly, instead of resolving it from the DI container.
The underlying ConfigureBuilderContextExtensions.EnableReloads already accepts the monitor directly - this change surfaces that path through the public API.
Use cases enabled:
Confirm the following