Add extension methods for creating OptionsBuilder with ValidateOnStart support#89973
Add extension methods for creating OptionsBuilder with ValidateOnStart support#89973steveharter merged 2 commits intodotnet:mainfrom
Conversation
|
Note regarding the This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change. |
|
Tagging subscribers to this area: @dotnet/area-extensions-options Issue DetailsFixes #89263
|
|
CC @halter73 |
|
CC @ericstj |
src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Options/src/OptionsServiceCollectionExtensions.cs
Show resolved
Hide resolved
halter73
left a comment
halter73
left a comment
There was a problem hiding this comment.
We technically approved adding this to OptionsBuilderExtensions instead of OptionsServiceCollectionExtensions in API review for some reason, but OptionsServiceCollectionExtensions is the correct place.
| string? name = null) | ||
| where TOptions : class | ||
| { | ||
| return new OptionsBuilder<TOptions>(services, name ?? Options.Options.DefaultName).ValidateOnStart(); |
There was a problem hiding this comment.
Shouldn't this call AddOptions? I think neither the OptionsBuilder constructor nor ValidateOnStart do this.
| return new OptionsBuilder<TOptions>(services, name ?? Options.Options.DefaultName).ValidateOnStart(); | |
| services.AddOptions(); | |
| return new OptionsBuilder<TOptions>(services, name ?? Options.Options.DefaultName).ValidateOnStart(); |
There was a problem hiding this comment.
ValidateOnStart() does call optionsBuilder.Services.AddOptions<StartupValidatorOptions>() which calls services.AddOptions();
| /// <param name="name">The name of the options instance.</param> | ||
| /// <returns>The <see cref="IServiceCollection"/> so that additional calls can be chained.</returns> | ||
| public static OptionsBuilder<TOptions> AddOptionsWithValidateOnStart< | ||
| [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] TOptions, |
There was a problem hiding this comment.
Why does this need to be .All? Shouldn't it be
[DynamicallyAccessedMembers(Options.DynamicallyAccessedMembers)] TOptions,?
There was a problem hiding this comment.
The same question for the other overload as well. I believe it should be:
public static OptionsBuilder<TOptions> AddOptionsWithValidateOnStart<
[DynamicallyAccessedMembers(Options.DynamicallyAccessedMembers)] TOptions>(
this IServiceCollection services,
string? name = null)
ghost
locked as resolved and limited conversation to collaborators
5 participants
Fixes #89263