Skip to content
This repository has been archived by the owner on Nov 7, 2018. It is now read-only.

Add Configure<TOption, TDep1, TDep2> sugar #239

Closed
wants to merge 4 commits into from
Closed

Add Configure<TOption, TDep1, TDep2> sugar #239

wants to merge 4 commits into from

Conversation

HaoK
Copy link
Member

@HaoK HaoK commented Oct 16, 2017

For #236

Making sure the current pattern looks good before adding all of the rest boilderplate (will also add matching PostConfigure versions...

Basic idea will be to enable: (up to 5??)

These register a transient func that just uses GetRequiredService for each Dep, its up to the caller to add the deps themselves with whatever lifetime they want.

services.Configure<TOptions, Dep1, Dep2, Dep3, Dep4, Dep5>("namedOption", o,d1,d2,d3,d4,d5 => o.Something = d1.A + d2.B + d3.C + d4.D + d5.E)

@davidfowl @ajcvickers

@khellang
Copy link

Is there a reason you won't just add a metod taking Action<TOptions, IServiceProvider> that gets wrapped in an IConfigureOptions<TOptions> and registered in the container?

@davidfowl
Copy link
Member

Is there a reason you won't just add a metod taking Action<TOptions, IServiceProvider> that gets wrapped in an IConfigureOptions and registered in the container?

We should just do both as part of this PR. May as well bring this back #152.

/// Implementation of IConfigureOptions.
/// </summary>
/// <typeparam name="TOptions"></typeparam>
/// <typeparam name="TDep"></typeparam>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dep1, Dep2...5

@HaoK
Copy link
Member Author

HaoK commented Nov 7, 2017

Updated PR adding Configure and PostConfigure methods taking 0 to 5 dependencies. Also added OptionsBuilder<TOptions> to potentially be the place to hang these overloads... so the builder binds to a specific named instance for chaining purposes.

Examples:

            var default = services.BuildOptions<FakeOptions>()
                   .Configure<SomeService>((o,s) => o.Message = s.Stuff);

            var named = services.BuildOptions<FakeOptions>("named")
                   .Configure<SomeService>((o, s) => o.Message = "named "+s.Stuff);

            // Future framework methods can also expose their options builders
            var xyzOptionsBuilder = services.AddXyz(o => o.Set = "Something")
                    .Configure(o, s => o.Other= s.Compute())

In terms of naming, we could go with services.AddOptions<TOptions> instead of Build since we currently don't use that overload

@jdom
Copy link
Contributor

jdom commented Nov 7, 2017

I do think that the OptionsBuilder is the right place to put all of these extensions. I would keep the original Configure<TOptions> on IServiceCollection, but only for back compatibility, but in theory they could just forward to this. The only ones that don't really fit in this builder are the ConfigureAll extension methods, as these apply to all, and not just to a named option.

BTW, as I mentioned in #238 (comment), I'm not that fond of calling it Builder or use Build in the name (as I originally did), as it is not really building something isolated from IServiceCollection, but instead it's helping configure the collection**. I think OptionsConfigurator is a better name for the class, but haven't decided on the extension method. Maybe AddOptions<T> as you suggested is good. The ideal would be ConfigureOptions<T>, but that's already taken.

** what I mean it's not building in isolation, is that these 2 calls would effectively configure the same options one after the other, and not build 2 different instances of the options, of course:

services.BuildOptions<FakeOptions>().Configure(o => o.Message = "1");
// ... 
services.BuildOptions<FakeOptions>().Configure(o => o.Message += "2");

@HaoK
Copy link
Member Author

HaoK commented Nov 7, 2017

Yeah AddOptions might work better since it behaves like the other AddXyz()'s today where if you call it twice the options manipulations are just run in order:

            var default = services.AddOptions<FakeOptions>()
                   .Configure<SomeService>((o,s) => o.Message = s.Stuff);

            var named = services.AddOptions<FakeOptions>("named")
                   .Configure<SomeService>((o, s) => o.Message = "named "+s.Stuff);

@jdom
Copy link
Contributor

jdom commented Nov 7, 2017

Cool, I'll update my OptionsBuilder PR with the new names:

OptionsBuilder => OptionsConfigurator
BuildOptions<TOptions> extension method => AddOptions<TOptions> extension method

I will also include the PostConfigure method that I left out for my initial PR. Never mind, I realize I did include it in my original PR already.

@HaoK
Copy link
Member Author

HaoK commented Nov 7, 2017

We can leave the class name alone, we already have this pattern every where (MvcBuilder/IdentityBuilder/AuthenticationBuilder), all of them are basically adding sugar methods for manipulating the IServiceCollection

@jdom
Copy link
Contributor

jdom commented Nov 7, 2017

Hah, OK, I'll revert that commit then, I was about to push :)

@HaoK HaoK changed the title [Prototype] Add Configure<TOption, TDep1, TDep2> sugar Add Configure<TOption, TDep1, TDep2> sugar Nov 13, 2017
@HaoK
Copy link
Member Author

HaoK commented Nov 13, 2017

This should be ready to review now for real, builds on top of the new OptionsBuilder we added via @jdom 's PR:

  • [Post]Configure<D1...D5> were added to IOptionsBuilder
  • Test added to ensure IServiceProvider works as a dependency as well

@ghost ghost removed the cla-already-signed label Nov 14, 2017
@HaoK
Copy link
Member Author

HaoK commented Nov 21, 2017

edc21af

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants