List and Dictionary binding #59
Conversation
ghost
added
the
|
Btw, Travis and AppVeyor will fail without my changes from Configuration. So don't panic :) |
| return; | ||
| } | ||
|
|
||
| VisitType(property.PropertyType, ref propertyValue); |
There was a problem hiding this comment.
nit: this is really VisitProperty right?
There was a problem hiding this comment.
Or more specifically, Read/GetPropertyValue? Is there a reason this method is void with ref object instead of just returning the propertyValue?
There was a problem hiding this comment.
Ref is needed because you can have a get only property that was initialized previously and we want to be able to the bindings on that instance. So, it will use the existing instance if it exists, otherwise it creates a new one
There was a problem hiding this comment.
It could simply return the value but I think it is more explicit to have a ref parameter. It tells you that we can change that value
There was a problem hiding this comment.
Why can't you just return the reference to the previous instance or the new instance?
There was a problem hiding this comment.
Yeah I think that is normally understood that objects are mutable. I don't think we typically use ref parameters in general :) It definitely sticks out a bit like a sore thumb... I'd just call it out in a comment for the method if you want to emphasize this point. Making the signature void/ref is overkill (especially since this is a private method)...
|
Should we consider moving the ConfigurationBinder to configuration itself? For full disclosure, when I asked @lodejard this way back when I first wrote this, his reasoning was having the binding functionality live in options, rather than config nudges people towards options instead of directly binding against configuration... |
| new ConfigurationBinder(configuration).Read(merge); | ||
| } | ||
|
|
||
| public void Read(object merge) |
There was a problem hiding this comment.
Perhaps Bind(object instance) would be more clear?
There was a problem hiding this comment.
That's a good suggestion
victorhurdugaci
force-pushed
the
victorhu/arrays
branch
from
8013a99
to
f801a76
Compare
|
Updated the PR
mmm... maybe? I see the point of having bindings directly in configuration but at the same time having it in options makes sense too. Maybe it would make sense to merge options and configuration completely... |
| _startConfiguration = configuration; | ||
| } | ||
|
|
||
| public static void Bind(IConfiguration configuration, object merge) |
|
Updated |
|
|
||
| namespace Microsoft.Framework.OptionsModel | ||
| { | ||
| public class ConfigurationBinder |
There was a problem hiding this comment.
This whole class pattern is a bit unusual... almost all the methods are static, except for the ctor and one method, and the only field is set once in the ctor and used once in that one method. There don't seem to be any cases where the instance Bind method is called more than once. The runtime doesn't seem to do it, the unit tests don't do it. Is that a scenario?
There was a problem hiding this comment.
There was a problem hiding this comment.
I could see potentially being entirely non-static if it helps unit testing, but as it is right now it seems a bit contrived.
There was a problem hiding this comment.
Changed to static
- Support for binding to dictionary
victorhurdugaci
force-pushed
the
victorhu/arrays
branch
from
125231e
to
2f4d735
Compare
|
I am still not sure how to read an Array of objects in program.cs file using IConfigurationRoot or ConfigurationBuilder. Does anyone have example for the same? |
Fixes: #56
Please review: @lodejard @HaoK @glennc