ConfigurationBinder Bind to support scalar value, added new Get<T>

[MichaCo]

• New Get and Get extension methods
• Binding scalars works with all methods now in addition to POCOs.
• New unit tests
  Consider having sugar methods for type conversion of values in ConfigurationBinder #281

[HaoK]

nit: you should use named parameters when passing null

[HaoK]

Looks good to me, @divega thoughts?

[divega]

Looks good in general. I have comments but I think they apply to the existing code even more than to the PR:

• I think we should decide on what behaviors we want for missing/null parameters. Then we should probably add some parameter checks based on that, but not necessarily everywhere, e.g. we might decide that in some scenarios the API will become a no-op if you don't pass some of them.

• I see some naming issues:

  • For me model as a parameter name on the public API doesn't make much sense outside of MVC model binding.
  • As someone looking at the code to try to understand it, it is hard to guess what the difference is between BindObject() and BindInstance(). This one is not public API so it is just internal code quality.
  • I am not sure why BindInstance() ignores the passed object some times. It might be intentional and an implementation detail but it smells to me.

• I have been looking at how this API could be used and I think we should consider accepting default values, e.g.:

  var timeout = config.Get<int>("timeout", defaultValue: 30);
  // or even simpler, inferring the type from the default value:
  var timeout = config.Get("timeout", defaultValue: 30);

  Some users would prefer to add those default values in an in-memory configuration source so that they always get returned by the IConfiguration, but the pattern above seems to be less hassle.

[divega]

And regarding the last bullet on default values, what happens now if I ask for e.g. an int and:

1. The value found is null?
2. The value found is not null but cannot be converted to int?

(sorry if I am being too lazy to read the tests or try it 😄)

[MichaCo]

@divega I can rename model to obj for bind, any better idea for the name? ;)

BindObject -> I thought its the best name because it is only binding all properties of the object which gets passed in.

Regarding BindInstance, it ignores the object in case the method should return the value of a section already. And if the object passed in is null, it tries to instantiate it and then calls the different bind methods. This method does a lot of different stuff, maybe it could be refactored into multiple methods, or we just rename it. Any idea?

Regarding the default values, I agree that this would be nice, but I guess this should be another PR and Task, right?

[divega]

I can rename model to obj for bind, any better idea for the name? ;)

I would suggest using instance for this. It is practically a synonym of object, which is why it is so confusing to have a method called BindObject() and another one called BindInstance() in the implementation 😄

BindObject

What about BindNonScalar, BindStructuralObject or BindObjectProperties? IMO something that conveys that this is for the path in which we didn't bind to a value is what we need.

BindInstance ... refactored

Don't worry about that for now.

default values...

Agreed it should be a different PR. I will file a bunch of new bugs for these and some other concerns I have.

[divega]

Filed #285 and #286. @HaoK can you do a final review to see if this is ready?

[MichaCo]

@HaoK I fixed your PR comments and also added a few more unit tests
@divega

what happens now if I ask for e.g. an int and
The value found is null?

I actually found a bug in Get<T> when BindInstance returns null, the cast to (T) of course throws a null reference exception. This is fixed now and has a unit test.

In general, if the value found is null, we will return default(T), see unit tests.
That being said, in case of collections, the value will not be added to the list! see unit tests

The value found is not null but cannot be converted to int?

That's already being tested, Get will throw the same exception as Bind did before all the changes.

There is still one possible null reference exception in the code which is if the configuration parameter on Bind or Get/Get<T> is null. This could happen if someone calls ConfigurationBinder.Bind directly. In this case, BindInstance will throw the exception...
I think we should check for null in BindInstance and return the passed in instance/null. The same happens if config.GetChildren().Any() is false anyways.

[divega]

I think we should check for null in BindInstance and return the passed in instance/null. The same happens if config.GetChildren().Any() is false anyways.

Sounds good, however I would expect the public methods to throw if a null IConfiguration is passed at the start. Thoughts?

[MichaCo]

Yup makes sense, should I add the [NotNull] attribute or check for null and throw an ArgumentException directly?

[divega]

Both. We don't know if we'll get to finish the code that auto-generates the checks based on the attribute.

[MichaCo]

Ok that's done, too ;)

[HaoK]

Re [NotNull] my preference is to just use the attribute for new code which seems to be the pattern in most places.

[divega]

@HaoK actually you need to change your preference 😄

[divega]

Use nameof(configuration)

[divega]

@HaoK was right re [NotNull]. It seems we are not using it consistently in this repo so there is very low value in adding it for this methods. On the other hand it doesn't hurt so no need to remove it from the PR either.

In general LGTM with minor feedback. @HaoK want to do another last review? 😄

[MichaCo]

@divega Your comments are fixed, too

[divega]

LGTM. @HaoK please finish reviewing and take this. We might need to rebase with #288.

[HaoK]

Only minor thing is can we nuke the [NotNull] since we are removing them everywhere, lets not add more now

[MichaCo]

I removed the [NotNull] and squashed everything into one commit. Didn't merge yet, let me know if you want me to do that

[HaoK]

Looks good to me, merge away

[divega]

Thanks for your contribution @MichaCo!

[HaoK]

Hrm now we throw on Core:

Microsoft.Framework.Configuration.Binder.Test.ConfigurationBinderTests.GetUri [FAIL]
System.InvalidOperationException : Failed to convert 'http://www.bing.com' to type 'System.Uri'.

Microsoft.Framework.Configuration.Binder.Test.ConfigurationBinderTests.CanReadAllSupportedTypes(value: "http://www.bing.com", type: typeof(System.Uri)) [FAIL]
System.NotSupportedException : TypeConverter cannot convert from System.String.

[HaoK]

I'll just disable these two new tests on core since it doesn't appear to be supported

[MichaCo]

@divega welcome ;)
@HaoK oh wow sorry about that! I was pretty sure I ran tests on both clr and coreclr.
Maybe you should add an issue to the https://github.com/dotnet/corefx repo?