replace earlier binding infrastructure #408
Conversation
jonsequitur
force-pushed
the
decompose-ReflectionBinder
branch
from
928f0c0 to
22fdb6c
Compare
| return GetValues(context, ConstructorDescriptor.ParameterDescriptors); | ||
| } | ||
|
|
||
| public object CreateInstance(BindingContext context) |
There was a problem hiding this comment.
Nit, consider GetInstance
There was a problem hiding this comment.
Can you explain how you think about the distinction here?
There was a problem hiding this comment.
On second look, Create is fine
| new SpecificSymbolValueSource(option)); | ||
| } | ||
|
|
||
| public void BindMemberFromCommand<TValue>( |
There was a problem hiding this comment.
Does this mean BindMemberFromArgument? How will a tight coupling between commands and arguments here work with multiple arguments? I do not believe people think about commands having values as much as having arguments.
There was a problem hiding this comment.
Ignore the above because this isn't the place for it, but to clarify the confusion.
Our implementation is at odds with the ways I believe people think about the problem, and reflects an implementation detail. I do not believe people say "What's the value of that command?" Nor "What's the value of the argument on that option?" I think they would say "What's the value of the argument?" or "What's the value of the option?" and that is how Help seems laid out to me. I think this is going to be a rough cognitive load and reflects an implementation detail, and I know I will keep making this mistake whenever I am thinking about the real problem (the CLI).
But this has nothing to do with the problem we are trying to solve today.
There was a problem hiding this comment.
It does mean bind from the argument. The concern is a good one. I think conceptually "What is the value for this option" is more intuitive but less precise than "What is the value for this option's argument"? And yes, the value for a command will become more ambiguous with #310.
|
|
||
| namespace System.CommandLine.Binding | ||
| { | ||
| public class ModelDescriptor |
There was a problem hiding this comment.
AppModels will need to set just some properties. I think this will be a need for some other cases as well. How would you pass the list of PropertyDescriptors to this class?
There was a problem hiding this comment.
Do you mean set some properties that were not discovered via reflection?
|
|
||
| public bool HasDefaultValue => false; | ||
|
|
||
| public object GetDefaultValue() => null; |
There was a problem hiding this comment.
People may be confused by this - properties now appear to have default values. Not sure if we can work with them.
There was a problem hiding this comment.
This isn't intended to being limited the capabilities of reflection. The intention is to be able to annotate the model descriptor so that, for example, if you know that a given type behaviorally has a default, you can define that in the model.
|
|
||
| public bool HasDefaultValue => false; | ||
|
|
||
| public object GetDefaultValue() => null; |
There was a problem hiding this comment.
Consider returning the type default instead of null.
jonsequitur
force-pushed
the
decompose-ReflectionBinder
branch
from
853e3ed to
6eabdda
Compare
jonsequitur
force-pushed
the
decompose-ReflectionBinder
branch
3 times, most recently
from
573c2ee to
0693638
Compare
|
@dotnet-bot test Ubuntu16.04 Release |
jonsequitur
force-pushed
the
decompose-ReflectionBinder
branch
from
0693638 to
87abf71
Compare
…cts to DragonFruit
jonsequitur
force-pushed
the
decompose-ReflectionBinder
branch
from
87abf71 to
10c374c
Compare
jonsequitur
changed the title
|
@dotnet-bot test Ubuntu16.04 Release |
This is attempt #3 at a generalized model binding infrastructure with lessons learned from the previous two iterations. If we think this is a good approach, this is intended to replace the following types and their related code:
TypeBinderMethodBinderReflectionBinderThis is intended to address the following scenarios in a way that is flexible enough to be leveraged within app models, e.g. DragonFruit:
ParseResult, service provider, arbitrary inputs such as environment variables, etc.) to arbitrary types (including constructor parameters, property setters, and handler parameters)It also shifts the code for building options based on a method signature, which DragonFruit uses, into
System.CommandLine.DragonFruit. Some of this may later move under the app model work but as it's highly convention-based, we decided that it does not belong in the core library.