Options binding from configuration should not have conditionally compiled code

[divega]

Update: Covert.ChangeType() is now available in System.Runtime.Extensions and we can remove the conditional compilation.

Currently the whole body of OptionsServices.ReadProperties(object, IConfiguration) is in an #if and won't do anything if the functionality of Convert.ChangeType() presumably isn't available in the target platform. I believe we should decide more explicitly what we want to do with this, e.g.:

1. Throw an exception
2. Make sure the needed functionality is made available in CoreCLR and use it

[Eilon]

I think @yishaigalatzer wanted to kill that whole type conversion feature for now and just do strings.

Do we have any components that truly rely on the type conversion behavior of Options / config? Does EF make use of it?

[divega]

Updating title because Covert.ChangeType() is now available in System.Runtime.Extensions and we can remove the ifdefs.

[divega]

@Eilon in general our Options classes are optimized for code configuration and they are not just for pulling settings from configuration, so not sure it is a good idea to kill the type conversion/binding. Was there any scenario in which it was dangerous or didn't work?

[Eilon]

For code-based config you don't need type conversion right?

I think the only danger in the current state of the code is that it ignores stuff instead of either working or at least failing. I'm fine if we can do more basic conversions successfully, but then we should fail if we can't. That's probably a decent compromise and easy to implement, right?

[divega]

Yep, we should not swallow exceptions from Covert.ChangType(). Frameworks that need more complex conversions/parsing should implement their own logic. @HaoK sure has more context on why the empty catch ended there.

On the other hand is by design that we skip properties not specified on configuration.

[Eilon]

Skipping missing values is certainly fine (if I understood correctly). I mean ultimately we'll add new options and wouldn't expect "old" configs to start to fail.

[HaoK]

I can take a look at this again, it was added very early on and since nothing was really using it, I didn't flesh the type conversion logic out much at all

[yishaigalatzer]

My key feedback here was that the data should start with some well known subkey otherwise random collisions will start happenning

[Eilon]

The user passes in an optional sub-key when they use config, no?

[HaoK]

Yeah the caller is responsible for selecting the right parent config for the options to bind against, i.e.

services.ConfigureMvc(config.GetSubKey("mvc"))

[Eilon]

Yeah I think that's fine by me. Forcing a parent key was discussed a long time ago and was determined to be far too difficult to use. If we had a default we'd need a way to un-set it, we'd need a way to document it, we'd still have collisions (though rare), and we'd still have to support custom sub-keys anyway.

[lodejard]

Tangent - I wonder if GetSubKey should be GetKey?

[yishaigalatzer]

The fact that you pass configuration rather than something like key or configuration node, confused me I want even aware of the pattern. From the shape of the API it looks like you just pass the configuration object proper. Consider making it more obvious, such that the API is self explanatory

[HaoK]

Well the configuration itself is a node so it's just the nature of the new iconfiguration that makes it flexible. Config.GetSubkey returns another iconfiguration representing the root of the subtree.