BCL API review: ConfigurationBinder feedback

[divega]

The ConfigurationBinder static class is considered DNX specific and embodies the ability to apply configuration values from an IConfiguration to the matching properties of an instance of a simple class. It is used in the implementation of Options.

Currently there are two public Bind methods in it:

public static TModel Bind<TModel>(IConfiguration configuration) where TModel : new()

public static void Bind(object model, IConfiguration configuration)

The methods have different purposes, e.g. the generic version will create an object of type TModel while the no-generic one receives the object. The implementation of the latter no-ops if the objects passed is null.

We want to rationalize this API, including revisiting the names of the class and methods and also the signatures and the behavior of the methods. After gathering feedback from the BCL team and then having some discussions on our own we have come up with this API:

public static TModel Bind<TModel>(this IConfiguration configuration) where TModel : class, new()
public static TModel Bind<TModel>(this IConfiguration configuration, TModel model)  where TModel : class

// Usage:

var orgSettings = configuration.GetSection("Org").Bind<OrgSettings>();
var tenantSettings = configuration.GetSection("Tenant1").Bind(new AppSettings());

Notice that:

1. Bind overloads are extension methods on configuration
2. They return the instance of TModel, regarless of whether it got passed or created by the call.
3. They don't support structs.
4. The overloads that take a TModel argument should throw if a null model is passed.

[HaoK]

👍 I didn't like where we ended up with the names on these either

[divega]

I have updated the API based on a discussion we had today with @HaoK, @lodejard and @kirthik. At the end of it I think we had tried too many different variations of the methods to keep a sense of what is good 😄, so we might need to do another pass to clean it up. E.g. I would propose that it would be enough to have this overload to cover all the cases without adding much friction:

public static TModel Bind<TModel>(this IConfiguration configuration, TModel model) 
  where TModel : class

// Usage:
var appSettings = configuration.Bind(new AppSettings(), );
var tenantSettings = configuration.GetSection("Tenant1").Bind(new AppSettings());

[HaoK]

That was my initial response too, just make them filter the configuration before calling bind. The sugar doesn't get you all that much

[HaoK]

I thought I was outvoted 2:1 so I didn't push, but if its 2:1 against, I'll push :)

[divega]

Yeah, same for the one that creates the instance (but forces you to specify the generic argument). I am not all convinced about the merits of ApplyTo over Bind or other names we discussed either.

[HaoK]

That's fine, I'll send out a PR, can pick a final name during that process, might help to see it used everywhere

[HaoK]

Usage is mostly contained to Options right now I think

[HaoK]

@divega is this ok to bump to beta 8 or do you want to me to slam it in ?

[divega]

The term "slam" seems to imply it could be messy in some way if we try to get it in for beta 7. Is it just the churn of making cross repo changes?

[HaoK]

I use slam when its highly likely the build will break since I don't have time to do it in a careful way, would basically push the PR and fix repos as they break on the CI... I don't know how widespread the change is, might not be too bad, but that's what I mean by slam... I have a lot of changes in a lot of repos going on, but I don't THINK this one is too bad, why I offered to slam it in :)

[HaoK]

This looks like its just minor cleanup that should only affect binder, so it doesn't seem that high prioirty to do now, so we could just do this for beta 8 too... why I asked you what you think we should do with this one

[HaoK]

Take a look at the PR: #273 and if you ship the squirrel, I'll slam it in tomorrow :)

[divega]

OK. Will do. BTW, I had one more thought about the overload that creates an instance when I realized a few days ago that ConfigurationBinder will activate nested instances. Will comment in the PR as appropriate, but I think we should probably keep it.

[divega]

Updated the original issue to reflect some more recent thinking.

[HaoK]

So some suggested tweaks: the generic should only be on the overload that creates and has the new() constraint. The generic and return value only make it hard to use the Bind overload that takes an instance, as now you can longer just pass in anything have it work. You have to specify the generic argument to call it which makes the method painful to use in some situations.

I recommend instead this:

public static TModel Bind<TModel>(this IConfiguration configuration) where TModel : class, new()
public static void Bind(this IConfiguration configuration, object model)

[HaoK]

And personally I still argue infavor of killing the overload that is just moving the new TModel inside of the method. I don't think it adds anything really

[HaoK]

28b62f0