Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Start discussing design around dynamic commands and command providers #195
base: master
Are you sure you want to change the base?
Start discussing design around dynamic commands and command providers #195
Changes from all commits
dc13c33
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it looks like
ICommandType
isCommandSchema
andIExecutableCommandType
isICommand
. Why we have inheritance here? These are two separate things, and I don't like a concept of mixing them. If someone wants "Typin reflection" aka schemas, there is a service for this.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with my previous comment above this was made clear. We could even skip the distinction and make a
ICommandSchema
returnnull
whenCreateRunner
is called to indicate that there is nothing to run, or have that information explicitly in theCommandDescriptor
with a propertybool IsAbstract
. This is a detail for later.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how will you know
CommandArgumentDescriptor
akaCommandParameterSchema/CommandOptionSchema
while parsing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I created the class
CommandRunnerContext
which is the context in which current command is running, and that context is prepared after converting theIEnumerable<string>
, using the CommandDescriptor that corresponds to the typed command, into a parsed command, which know theCommandDescriptor
and all theCommandParameterDescriptor
andCommandOptionDescriptor
used to parse the command line.So the
ICommandRunner
, instead of receiving aICliContext
, would receive a more specialized context with more information after the parsing have happened.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why I created the class
CommandRunnerContext(...)
-ICliContext
/CliContext
does exactly the same :) However, maybeICliContext
/CliContext
design is not 100% ideal. But I'm planning to change it in Typin 4.0 due to GenericHost usage, and different middlewarre configuration (more like what we have in ASP.NET Core)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, maybe not exactly. We have just a raw or partially parsed input - CliContext lack a relation between input and ICommand arguments (some intermediate output from binder).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to follow the basics of ASP.NET filters, where you have access to
HttpContext
, but also to a specific context to the filter at hand, where you have other processed data. like thrown exception,ActionDescriptor
, etc.So you have
HttpContext
(like ICliContext) andExceptionContext
,ActionExecutingContext
and others, representing current command execution (with parsed arguments, and command metadata).Maybe we can design it differently, but I just laid out a first approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I know you didn't want to reuse any of your types to be as succinct as possible, but this makes everything so much harder to understand. :<
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I can hope for a better understanding after a couple of iterations. If you have any idea how to converge to less friction, lets talk about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is why I think that this changes are more aligned with a next version of the library, because I'm proposing a lot of changes. We can incorporate some of these to current 3.x version, and evaluate on that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have amazingly thought-provoking and interesting ideas of new mechanisms. I'm thinking of writing a new concept/draft of dynamic commands myself to show my point of view, so that we can later combine what is best in our two visions. I think this will also help with disussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be great! I prefer playing with the design until it clicks, and then start building.
Also I have shown a lot of mini features but lets focus on the feature at hand: Dynamic Commands