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

Add support for binder to create models #467

Closed
tillig opened this issue Jun 15, 2016 · 15 comments
Closed

Add support for binder to create models #467

tillig opened this issue Jun 15, 2016 · 15 comments

Comments

@tillig
Copy link

tillig commented Jun 15, 2016

Currently if you want to bind configuration to any sort of user-defined model, you have to create the model and then bind:

var model = new MyModel();
configuration.Bind(model);

You have to do that because GetValue<T> only uses type conversion to convert the value in a specific key into a destination type. So something like this works...

var enabled = configuration.GetValue<bool>("enabled");

...but something like this doesn't work:

// Always returns null.
var model = configuration.GetValue<MyModel>("settings");

This becomes really painful if you want to bind an array of settings back into an array. If I want an enumerated list of settings, it has to be like:

var settings = new List<string>();
configuration.Bind(settings);

Other similar scenarios become multi-step pain, too. Instead of only using type conversion in the GetValue<T> mechanism, it would be nice if that process would also make use of the (private) CreateInstance method in the binder already and create a new version of the model/bind that new version if possible.

@HaoK
Copy link
Member

HaoK commented Jun 15, 2016

You can think of GetValue as more of a Convert<T> as its just using TypeConverter the string value to T (which is mostly value types) as things currently stand, its not really model binder related anymore.

I had proposed moving it out of the package and into general config for this reason and to make it less confusing, but we didn't want to add the component model dependencies.

My personal opinion is that there would be value in a universal Get<T> like we had before that 'just does the right thing', but you'll have to help me convince @divega :)

@tillig
Copy link
Author

tillig commented Jun 15, 2016

I don't have a huge preference as to whether there's "one method to rule them all" vs. separate methods for each thing, but I figured GetValue<T> seemed like as good a place as any. I suppose the other option if multiple methods still exist would be to have a Bind<T>() overload that doesn't require I pass in an existing model.

@divega
Copy link

divega commented Jun 15, 2016

GetValue<T>() is a very simple API that only works with configuration values, i.e. primitive or scalar values associated with a configuration key. Bind() on the other hand is about taking whole sections of configuration and trying to bind them to structural objects based on key to property name matching. We can add more convenient versions of what Bind() does in the future. We can also move the APIs around to different packages as long as we don't make breaking changes in a version that shouldn't have breaking changes.

@divega divega changed the title Add support for binder to create models on GetValue<T> Add support for binder to create models Jun 15, 2016
@HaoK
Copy link
Member

HaoK commented Jun 15, 2016

Yeah I think the confusion mostly lies around GetValue living in the binder. If it lived somewhere else, I don't think it would be as confusing. Or if it was called ConvertValue.

@tillig
Copy link
Author

tillig commented Jun 15, 2016

I think ConvertValue would be the best solution. That way even if it's in the binder, it doesn't sound like it's going to do full binding. It's the combination of name + where it lives that was misleading... though I think if you moved GetValue<T> out of the binder it might still feel like it's going to do something more than just simple type conversion.

@HaoK
Copy link
Member

HaoK commented Jun 15, 2016

Note if we end up resurrecting a bind overload, we might as well bring it back as T Get<T> where T : new() like we had before.

@HaoK
Copy link
Member

HaoK commented Jun 15, 2016

Ship has unfortunately sailed for 1.0.0 for the name...

@divega
Copy link

divega commented Jun 15, 2016

@HaoK @tillig I guess we can all have different subjective perspectives of that is confusing and why. Trying to be objective, as @HaoK mentioned we are going to ship this as is in 1.0.0 and so I believe it may be more productive to think about:

  • How we do a better job of explaining what the differences are (e.g. by tweaking our various levels of documentation, writing samples, etc.)
  • What additive changes to the API we can make that will make it easier to use and understand. Breaking changes have a cost, and creating a new API that doesn't align with the concepts in the current API has the risk of creating more confusion instead of reducing it.

Back to my own subjective perspective:

  • I think that the Value part in the GetValue is a good hint of what the API does once you have some understanding of what "value" means in the Configuration system. The API is also an accessor pattern because it takes a key, so it is not just a "convert" operation.
  • Any auto-magical API that does either what GetValue<T>() does or what Bind() does depending on context will have to handle the ambiguity in the case that a that a configuration key path is associated with both a value and a group of sub-keys, i.e. a configuration section (I know @HaoK you don't think that is important but I disagree 😄). There are a few options to handle that ambiguity that could potentially work but in the end we decided that rather than doing something more complex it was an ok burden for the application to have to decide which of the two things (value or section) it really wanted to get. And at least this is the story in 1.0.0. It can evolve later.

@divega divega modified the milestone: Backlog Jun 15, 2016
@HaoK
Copy link
Member

HaoK commented Jun 15, 2016

My vote is to bring back the uber T Get<T>(string key) that just tries to 'do the right thing'.

The proposed behavior for IConfiguration.Get(string key) would be:

  1. Try to Convert (aka GetValue()) a non null section value to T. If successful return T.
  2. Otherwise try to Bind() the section against a new instance of T via reflection.
  3. Only return null if something catastrophic happens. (or throw)

If there's ambiguity somehow there, users can manually call Bind instead. But I'm still convinced Get will just work 95% of the time...

@tillig
Copy link
Author

tillig commented Jun 15, 2016

I liked Get<T> in earlier versions; losing that in RC2 is what got me here. 😉 I'd be happy if it were to return.

I get that name changes are out, and I'm totally fine with that. A clearer name for GetValue<T>would have been nice but isn't a deal-breaker. I do disagree that Value clearly indicates anything with respect to the behavior of the method, but that could just be me. Some slightly more explicit XML/Intellisense doc clarifying the difference may be good.

Gets a single configuration value and attempts to use type conversion on the value to coerce it into the specified type.

I'm primarily concerned about filling the gap (additive to the API? minor enhancement to existing API?) with some way to call Bind<T>(), GetValue<T>(), or even FizzBuzz<T>() to get the binder to create the model rather than having to bind to an existing model.

The largest confusion I had was that neither method had such functionality but by all accounts felt like it should have. GetValue<T> seemed to "create" the model (turns out to be type conversion rather than creation); Bind<T> has a way to create child entities in a model but not the top-level owner; and the gap between the two was where the confusion came in.

@HaoK
Copy link
Member

HaoK commented Jun 15, 2016

Yeah well that gap is where Get<T> used to live :)

The good news is if enough other people agree and also request it, we can probably bring it back in some form (it helps your cause that I never wanted to remove it)...

@MichaCo
Copy link
Contributor

MichaCo commented Jun 16, 2016

/missing the Get.
Totally get the concerns regarding the ambiguity. But I ran into that, too and was wondering why GetValue only works on the .Value object and why I have to write so much code now to get an object bound to a section? ;)

That being said, writing a custom extension yourself which uses Bind is probably a pretty easy option, too.

Maybe it would be more clear if we would have Bind<T> instead of Get<T>? (haha isn't that how it was in the very beginning ;) ?)

So, that Bind and Bind<T> clearly tries to bind the children of a section to a Poco, while GetValue tries to work with the section.Value only?

@divega
Copy link

divega commented Jun 16, 2016

@MichaCo yes, that is what I had in mind when I said we can create more convenient versions of Bind(), e.g. one that doesn't require an extra line to create the object beforehand. We can fill that gap without any drawbacks.

@HaoK HaoK added this to the 1.0.1 milestone Jul 19, 2016
@HaoK HaoK self-assigned this Jul 19, 2016
@HaoK
Copy link
Member

HaoK commented Sep 30, 2016

I went with T Bind<T>(this IConfiguration config) : where T:new() to start.

Do we need an overload that takes a string which is a shortcut for the common: config.GetSection(key).Bind<T>().

@HaoK
Copy link
Member

HaoK commented Oct 11, 2016

4686665

@HaoK HaoK closed this as completed Oct 11, 2016
@HaoK HaoK added 3 - Done and removed 2 - Working labels Oct 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants