Skip to content
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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

iskandersierra
Copy link

@adambajguz Lets start the discussion on this file.

I didn't want to reuse any of your types to be as succinct as possible. From here, once the design is clear we can start merging into your current solution.

@codecov
Copy link

codecov bot commented Apr 9, 2021

Codecov Report

Merging #195 (f9afcd1) into master (0d523d8) will decrease coverage by 0.99%.
The diff coverage is 0.00%.

❗ Current head f9afcd1 differs from pull request most recent head dc13c33. Consider uploading reports for the commit dc13c33 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #195      +/-   ##
==========================================
- Coverage   89.49%   88.50%   -0.99%     
==========================================
  Files         100      101       +1     
  Lines        2864     2896      +32     
  Branches      444      444              
==========================================
  Hits         2563     2563              
- Misses        209      241      +32     
  Partials       92       92              
Impacted Files Coverage Δ
src/Typin/Typin/Experimental/Design.cs 0.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0d523d8...dc13c33. Read the comment docs.

src/Typin/Typin/Experimental/Design.cs Show resolved Hide resolved
src/Typin/Typin/Experimental/Design.cs Show resolved Hide resolved
src/Typin/Typin/Experimental/Design.cs Show resolved Hide resolved
src/Typin/Typin/Experimental/Design.cs Show resolved Hide resolved
src/Typin/Typin/Experimental/Design.cs Show resolved Hide resolved
public string Value { get; init; }
}

public class ParsedOption
Copy link
Owner

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. :<

Copy link
Author

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.

Copy link
Author

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.

Copy link
Owner

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.

Copy link
Author

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

src/Typin/Typin/Experimental/Design.cs Show resolved Hide resolved
src/Typin/Typin/Experimental/Design.cs Show resolved Hide resolved

public class ParsedArgument
{
public CommandArgumentDescriptor Descriptor { get; init; }
Copy link
Owner

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 aka CommandParameterSchema/CommandOptionSchema while parsing.

Copy link
Author

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 the IEnumerable<string>, using the CommandDescriptor that corresponds to the typed command, into a parsed command, which know the CommandDescriptor and all the CommandParameterDescriptor and CommandOptionDescriptor used to parse the command line.

So the ICommandRunner, instead of receiving a ICliContext, would receive a more specialized context with more information after the parsing have happened.

Copy link
Owner

@adambajguz adambajguz Apr 11, 2021

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, maybe ICliContext/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)

Copy link
Owner

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).

Copy link
Author

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) and ExceptionContext, 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.

/// it's description from <see cref="ICommandType"/> and adds through <see cref="CreateRunner"/>
/// a way to obtain a new instance of a command runner.
/// </summary>
public interface IExecutableCommandType : ICommandType
Copy link
Owner

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 is CommandSchema and IExecutableCommandType is ICommand. 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.

Copy link
Author

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 return null when CreateRunner is called to indicate that there is nothing to run, or have that information explicitly in the CommandDescriptor with a property bool IsAbstract. This is a detail for later.

@adambajguz
Copy link
Owner

Hi @iskandersierra, just quick info: I don't think I'm going to have much spare time to work on Typin till June, but if you manage to write some code, I will find a few minutes for discussion/review :)

@iskandersierra
Copy link
Author

Hi @iskandersierra, just quick info: I don't think I'm going to have much spare time to work on Typin till June, but if you manage to write some code, I will find a few minutes for discussion/review :)

No worries, I'll try to do some prototyping. I'm also full of work, so you are very well understood.

@adambajguz
Copy link
Owner

Hi @iskandersierra, I have published a Typin 3.1.0 preview (Typin-3.1.0-dev-00014) with some changes I made 3 weeks ago - https://github.com/adambajguz/Typin/blob/develop/CHANGELOG.md

PS. Unfortunately nothing related to Dynamic Commands :< but there is a better custom prompt and custom type converter support :>

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants