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

How to dynamically create and remove commands? And other features requests #186

Open
iskandersierra opened this issue Apr 5, 2021 · 17 comments · Fixed by #207
Open

How to dynamically create and remove commands? And other features requests #186

iskandersierra opened this issue Apr 5, 2021 · 17 comments · Fixed by #207
Assignees
Labels
enhancement New feature or request Typin Typin library related issues
Milestone

Comments

@iskandersierra
Copy link

Is your feature request related to a problem? Please describe.
I would like to be able to add/remove commands dynamically from the app, depending on executions of previous commands (in interactive mode), to achieve something like http-repl tool in dotnet.

Describe the solution you'd like
I was checking your source code and I think getting the IRootSchemaAccessor and fiddling with Commands property is the way to go, but I just want confirmation from your part, if possible.

And once a command is added, how to include some help message, arguments/options descriptions (dynamically generated too)?

How to add multi-line content, like a HTTP POST content?

Describe alternatives you've considered
I havn't tried it yet, just wondering if this library can be a replacement for some internal cli-like tools we have, and this dynamic case is the Non Plus Ultra roadblock we get on every other solution out there.

Truth be told, I would prefer some kind of ICommandsProvider returning a collection of provided ICommands, and the RootSchema or the like, would Get an IEnumerable<ICommandsProvider> to obtain a composite collection of all the available commands. Some breaking changes might be required to achieve that, but it would add an extra layer of extensibility.

With this issue I just want to open a discussion around making this library more extensible. I'm willing to work on some features once well defined, if you agree.

@iskandersierra iskandersierra added the concept Help and further investigation on the proposal is needed. label Apr 5, 2021
@adambajguz adambajguz added the Typin Typin library related issues label Apr 5, 2021
@adambajguz
Copy link
Owner

Hi @iskandersierra , thanks for a great and inspiring concept.

Some of my first thoughts:

  • What is the purpose of dynamically adding, removing, and updating options and parameters? - I do think having separate commands for different scenarios is a much easier approach, but maybe I don't see something.
  • The destination I would really like Typin to evolve is GenericHost/TypinHost/CliHost with all schemas resolved on build time using source generators. So I don't really like the idea of adding, removing, and updating commands, options, and parameters; but what I thought about were schema areas or subschemas. So, you will be able to assign a command to an area/subschema and Typin will be able to restrict access to commands based on this. As programmers, we will be able to use e.g. IAreaSwitcher.
  • A specific example I thought about was some API that requires to be authenticated - in Typin words: successful execution of login command will give a user access to more commands, while logout will remove this access.
  • Suppose we have chosen the areas approach, we will probably also need to provide some logic to specify whether to inherit command to child areas. Areas will be a tree structure but one command may be in many areas - co maybe no inheritance to child areas needed - just explicit assign of command to area(s)?
  • Strongly typed areas and strongly typed switcher? a class per area with area parent(s) specification?
  • In direct mode areas/dynamic commands are pointless - how to have a simple direct mode and advanced interactive together?
  • "Some breaking changes might be required to achieve that (...)." - maybe that will be a perfect moment to move Interactive mode to separate NuGet package, e.g., Typin.Interactive? - or maybe not?

@adambajguz
Copy link
Owner

adambajguz commented Apr 5, 2021

... and one more:

  • The easiest solution is to make public IReadOnlyDictionary<string, CommandSchema> Commands { get; } in RootSchema modifiable but I'm not a fan of this.

@iskandersierra
Copy link
Author

Awesome response to know where you currently stand on the subject.

I'm thinking on the following use case:

> connect petstore --url https://localhost:8081/api --swagger https://localhost:8081/swagger/v1/swagger.yaml
# A new command named petstore is created and sub-commands are provided from the given Open API specification loaded at runtime

> connect tsi --url https://localhost:8080/api --swagger https://raw.githubusercontent.com/Azure/azure-rest-api-specs/master/specification/timeseriesinsights/data-plane/Microsoft.TimeSeriesInsights/stable/2020-07-31/timeseriesinsights.json
# Another command tsi is provided with a different specification and hence sub-commands

> help
Commands:
  connect       Connect to a REST API using a Open API specification
  dissconnect   Disconnect from a connected REST API
  petstore 		Access petstore API at https://localhost:8081/api (with Open API specification)
  tsi     		Access tsi API at https://localhost:8080/api (with Open API specification)
# Some of the listed commands were not known at build time
  
> petstore --help
Commands:
  pet			Everything about your pets
  store			Access to Petstore orders
# The list of sub-commands are generated from the Open API spec

> [>] petstore pet # I'm not used to the exact syntaxt to do a 'cd' command
petstore pet > --help
Commands:
  AddPet		Add new pet to the store inventory. POST /pet
  ListPets		List pets from the store inventory. GET /pet
  ...

petstore pet > ListPets -h
Options:
  --name <value>		Filter by pet name. ?name=<value>
  --species	<value>		Filter by species. ?species=<value>
  --output|-o			Output format: json, table, yaml
  --file|-f <file>		Output file
# Some options com from build time (output and file) and others come dynamically from the Open API spec

petstore pet > ListPets --name Fido -o table
...

My requirements are for cases when the command can only be configured from runtime information.

@iskandersierra
Copy link
Author

In fact, I think you are right to think that Interactive could be moved to it's own nuget, or even better, leave actual interactive mode to cover it's current scope (more build-time oriented), and have a sort of Typin.Repl nuget to cover a more advanced case of Read-Eval-Print Loop

@adambajguz
Copy link
Owner

Now I see your problem better - but i need to rethink it.

Leaving interactive mode to cover it's current scope is a good idea, but I'm not sure Typin.Repl is a good name. REPL is a programming environment. However, what I had for a long time in my mind, is a Batch mode - add commands to list (batch) then execute all of them with single command (event multiple times).

@adambajguz
Copy link
Owner

adambajguz commented Apr 5, 2021

Some implementation ideas/remarks:

  • Command schema with a bool IsDynamic prop.
  • Dynamic commands support restricted by CLI mode - dynamic commands as a core mechanism in Typin and Typin.Core libs.
  • IDynamicCommand interface to specify command logic - similar to ICommand but with extra param in ExecuteAsync to pass arguments dynamically.
  • Areas still can be implemented but this is a second feature/concept for interactive and batch modes. That in the future may be used with commands and dynamic commands.
  • Probably root schema should have a method to manipulate (add, remove) commands or commands collection should be mutable.
  • Probably validation needs to be rewritten because it is mainly in RootSchemaResolver but we will need it while adding/constructing a command. Also in for example CommandOptionAttribute there is some validation.
  • Fluent command builder?
  • As long as we use IRootSchemaAccessor help writers will work without any changes because attributes are only used for creating Schemas and these schemas are used by help, parsers, command execution.

Overall, it seems DynamicCommands is a quite easy concept to implement.

@adambajguz adambajguz added this to the v3.1 milestone Apr 5, 2021
@adambajguz adambajguz added enhancement New feature or request and removed concept Help and further investigation on the proposal is needed. labels Apr 5, 2021
@adambajguz
Copy link
Owner

adambajguz commented Apr 5, 2021

I created issues for Typin.Modes.Batch #190 and Typin.Modes.Interactive #191, as well as to analyze areas concept #187.

Going back to dynamic commands, I would be super happy if you start implementation (you have my full support, and I hope we will end up with a nicely implemented solution - that hopefully will be released in Typin 3.1 ;> ).

PS. What I also find out that to support DynamicCommands:

  • new prop bool IsDynamic must be also added to ArgumentSchema, and also a new constructor without PropertyInfo must be added.
  • then, probably a new binder DynamicCommandBinder will be used to convert input to concrete types, e.g. int, Guid if needed, and those binded types will be accessible through DynamicArguments class instance (that is passed to IDynamicCommand.ExecuteAsync) - I don't really know if we need concrete types or just strings (or both)?

@iskandersierra
Copy link
Author

If you think this idea fits what I'm describing, we can discuss it at your will, I can also help you if you need me to.

However having a single IDynamicCommand implementation in the project, does not allow for adding dynamic commands during runtime, only the dynamic processing of parameters. We would need a IDynamicCommandProvider, that would produce ICommand/IDynamicCommands when requested.

The mutability of the RootSchema is the easy answer, but it will be messy so I would avoid that can of worms. Instead a source of commands, where one source would be the current fixed lists of added commands, but as a user you could app.AddCommandProvider<OpenApiCommandProvide>() to add a new source of ICommands. The interface of ICommand would have to be augmented in order for the command to provide its metadata, and not just a way to be executed.

Something like (it might have typos):

public interface ICommand
{
    CommandMetadata Metadata { get; }

    IReadOnlyCollection<CommandArgument> Arguments { get; }

    IReadOnlyCollection<CommandOption> Options { get; }

    ValueTask ExecuteAsync(
        IConsole console,
        IReadOnlyCollection<object> arguments,
        IReadOnlyDictionary<string, object> options);
}

public record CommandMetadata(
    string Keyword,
    string Description // more metadata, like explicit examples, ...
);

Now you can offer a base class, like the following, to make easier to define commands like today with Typin:

public abstract class CommandBase : ICommand
{
    // Here implement ICommand explicitly to extract metadata, arguments and options using reflection from this.GetType()
    // And provide a SourceGenerator alternative to make it more performant and clean

    // Implementation of interface ExecuteAsync would fill the properties by reflection from the given arguments and properties
    // and call the simpler ExecuteAsync below

    protected abstract ValueTask ExecuteAsync(IConsole console);
}

Then a sample command could be implemented like:

// Using reflection

[CLICommand("sample", "Description")]
public partial class SampleCommand : CommandBase
{
    [CommandArgument(0)]
    public int MyArgument { get; set; }

    protected override async ValueTask ExecuteAsync(IConsole console) { ... }
}

// Using SourceGenerators

[CLICommand("sample", "Description")]
public partial class SampleCommand
{
    [CommandArgument(0)]
    public int MyArgument { get; set; }

    protected async ValueTask ExecuteAsync(IConsole console) { ... }
}

And going beyond, and this is taken from designs of our internal tools, I would like to be able to have a clise like the following:

[Command("sample", "Description")]
public class SampleCommands
{
    public SampleController(IService service)
    {
        this.service = service;
    }

    [SubCommand(Description = "List samples")] // Default route
    public ValueTask List(
        [CommandOption("name", "n")] string name,
        [CommandArgument(0, Description = "...")] FileInfo file,
        [ScopedService] IRepository repository,
        [ScopedService] IConsole console, // IConsole is just injected like any other service
        CancellationToken ct = default  // CancellationToken is "injected" from the scope, taken from the IConsole itself
    )
    {
        ...
    }

    [SubCommand("create", Description = "Create sample")] // Default route
    public ValueTask Create(...)
    {
        ...
    }
}

And then a call to app.AddCommandControllersFromThisAssembly() would add a corresponding ICommandProvider that generates all command from methods, instead of classes.

I'm sorry if I'm bombarding you with seemingly unconnected ideas :)

I you decide to ponder on them I can work with you to design and implement them.

@adambajguz
Copy link
Owner

adambajguz commented Apr 5, 2021

  1. The draft with SubCommand looks like some other CLI framework that I have seen some time ago, but I don't remember it's name - I don't want Typin to look like this. One class per command is nice and clean.

  2. Your draft looks like a complete Typin redesign - maybe it's needed or maybe not. I don't see pros and cons now. Also, I don't really get the idea of IDynamicCommandProvider. Is it for resolving dynamic command on Typin startup - before the first command is executed?

  3. public partial class SampleCommand - Do you want to build dynamic commands at compilation time?

  4. ICommandProvider is an interesting concept, then Typin may have swappable reflection-based and source generator-based providers.

I don't know whether we can have more providers for "standard" commands or not. Fluent builder/Expression based provider - strange? I'm also concerned about abusing this mechanism by Typin users - maybe this should be an internal mechanism - no user defined ICommandProviders.

EDIT: app.AddCommandControllersFromThisAssembly() - this is basically user-defined ICommandProvider aka (in current Typin's terminology) CommandResolver :D

  1. "However having a single IDynamicCommand implementation in the project, does not allow for adding dynamic commands during runtime, only the dynamic processing of parameters." - it doesn't need to be a single dynamic command in the app. We can have many of them - treat them as logic skeletons that encapsulate some concepts/features (one dynamic command for lists, one for get, another for paged lists, ....) .

  2. "The mutability of the RootSchema is the easy answer, but it will be messy so I would avoid that can of worms." - well I don't see this as a can of worms ;)

In my opinion, every command needs to be registered in RootSchema (central repository of all available commands - even if it is mutable)". A simple justification: autocomplete uses it as a source of commands, and also some future command suggestion system may also use it. Schemas are Typin's "reflection" - a window/insight to commands structure.

  1. "The interface of ICommand would have to be augmented in order for the command to provide its metadata, and not just a way to be executed." - I'd rather stay with splitting two concepts ICommand and IDynamicCommand and not messing in other users' heads.
  • Class implementing ICommand - basic, always fully defined command.
  • Class implementing IDynamicCommand - command skeleton that may have some fixed arguments, but is mainly using "dynamic arguments".

Probably IDynamicCommand : ICommend inheritance should exist if we go with "Option 1" from point 8.?

  1. Let's go back to the example with REST API client build from Swagger specification:

(sorry for typos and bad indent - written quickly in github editor)

[Command("connect")]
public class ConnectCommand : ICommand
{
      private readonly IDynamicCommandReposiotry _service;
      public ConnectCommand(IRootSchemaAccessor/IDynamicCommandRepository service)
      {
           _service = service;
      }

     public Task ExecuteAsync(IConsole console) //I know Cancellation token should be here - it will be in future -probably with an ability to cancel a single command and not terminate the whole app
     {
          //Get open api specification - http call
          
          //Build command
          //Note #1: command can be build in some user defined service
          //Note #2: while building dynamic command definition/schema we will provide a type of IDynamicCommand implementation class that will be a handler for this dynamic command

          CommandSchema dynamicCommandSchema = .........; //maybe we need DynamicCommandSchema : CommandSchema?

          _service.AddDynamicCommand(dynamicCommandSchema);
     }
}

public class ListDynamicCommand : IDynamicCommand
{
           //Option 1. init only props for arguments
           //Option 2. pass arguments to ExecuteAsync

           public Task ExecuteAsync(....) //
}

public class GetPagedDynamicCommand : IDynamicCommand
{
           //Option 1. init only props for arguments
           //Option 2. pass arguments to ExecuteAsync

           public Task ExecuteAsync(....) //
}

@adambajguz
Copy link
Owner

adambajguz commented Apr 5, 2021

  1. "However having a single IDynamicCommand implementation in the project, does not allow for adding dynamic commands during runtime, only the dynamic processing of parameters." - class generation at run-time? - sounds crazy

  2. We can think about commands from methods - just create a new issue, but I think we need to implemente ICommandProvider first. So let's focus on dynamic commands.

@iskandersierra
Copy link
Author

I propose I start working on the minimal changes so we can play with the ideas until it is just good enough to have a an iteration. I will fork the repo and start working on a draft PR so we can share ideas in the code.

1- The draft with SubCommands would be just another way (maybe given by an extension package) that would be possible with the ICommandProvider I'm thinking of. The Core would still be ICommand/IDynamicCommand and their providers

2- The idea behind ICommandProvider/IDynamicCommandProvider is to have an extensibility point to produce commands. In your current implemenmtation, RootSchema is the only provider of an immutable collection of commands (added through app.AddCommand). What I propose is to have RootSchema aggregate all ICommands (Lets not add IDirectiveProvider yet, but would be the same thinking behind it) provided by any instance of ICommandProvider. It would ask every time or just annotatate to events in the providers to know if the provider is indicating new commands are available. Some command providers would be boring like having a list of commands and others would load runtime specifications like Open API and build a new list of commands, now available from RootSchema.

3- public partial class SampleCommand - Do you want to build dynamic commands at compilation time? > I was just proposing different looks to see if they give some advantage over others. That's why I appologised about the bombardment of ideas. I'm brainstorming to see what could stick and be useful, and have all analysed by you too.

4- Yes, ICommandProvider in my opinion is the key to open a new extensibility point here. I'm thinking that this should be open to the client, but maybe in a namespace like Typin.Providers, where the normal use of the library would never go. Then we could have a different nuget, like Typin.CommandControllers, where a provider would allow us to create CommandControllers if that is the style of command the user want. By default, there would be a single ICommandProvider, just DefaultCommandProvider, and a single instance would be created by the app builder from the list of AddedCommands

5- In my example, when you run connect petstore ..., you would be creating a new "dynamic" command, if you run connect weatherapi ... again, you would be creating a second command. In fact, what you would be doing is communicating with a service to provide the connection configuration, and this service would provide new commands through the ICommandProvider implementation that is associated with this particular feature of providing commands from Open API specifications. So for this feature, you would do something like app.AddCommandProvider<OpenAPICommandProvider>() or app.AddOpenApiCommandProvider() and it would add commands connect and disconnect, and also add new command after the user interacted with connect

Later I'll keep commenting your issues. I'll have to get back to work :)

@adambajguz
Copy link
Owner

The more I think about these changes, the more I like them.

1,4. We may have NuGet packges: Typin.Providers.Default, Typin.Providers.CommandControllers, Typin.Providers.SubCommands, etc.

Typin.Providers.Default.Abstractions, Typin.Providers.SubCommands.Abstractions ?

2. let's leave dynamic directives for some much later iteration - I don't event know if dynamic directives are useful :)

" It would ask every time or just annotatate to events in the providers to know if the provider is indicating new commands are available." - I like this, especially events.

5. ok

@iskandersierra
Copy link
Author

Just to take it from where I left before:

6- As explained before, RootSchema would be the aggregate of all available ICommandProviders, so I don't see a need to mutate its collection of commands, It just would handle any event with new commands and update the collection itself.

7- Agreed that we have to rework a bit the interface hierarchy facing the user, to have it easy to use most of the times, and powerful enough for the complex cases

8- That's not what I have in mind, but again, we just need to converge to a common core framework, have it well designed to support the features we are talking about. As said before, I would do it the other way around, The RootSchema (or a RootSchemaHandler) would get injected IEnumerable<ICommandProvider> and then get annotated to event EventHandler<CommandsChangedEventArgs> CommandsChanged to keep updated the list of ICommands. We need to draft the interfaces and main components (as stubs and pseudocode) to agree on the design before starting to implement. As you can see this can be a rehaul on your current design, this could be an alphaof Typin 4.0 while you keep evolving 3.0. Maybe you see it differently, so lets talk about milestones too :)

9- It wouldn't be dynamic class generation. We would have a single OpenAPICommandProvider, and a single OpenAPICommand, that would be dynamic and configured at runtime, with potentially multiple instances representing different Open API specs. The user would register the provider and it would, though command OpenApiConnectCommand, talk to the provider to start providing a new instance of OpenAPICommand configured with the new swagger file. Then it would raise CommandsChanged event to notify RootSchema to reload the new command. Maybe using events is not the best solution, but that is a design detail for later.

10- As said before, a command provider from methods, would be just a different implementation of ICommandProvider, maybe at a different nuget package outside the core.

I just mentioned dynamic directives as a tangent, that would be a possibility. But I would event think about turning directives into commands (core commands that you could enable or not). I like command cd for this. It is very universal for those using CLI tools anyway. Something like:

> cd petstore pet
petstore pet > cd ..
petstore > cd /
> 

Let me know if this would be compatible with your current approach.

Other things I would like too is to be able to provide a custom Prompt, but it could collide with showing current path:

petstore pet > get --output table
... list of pets
petstore pet (68ms) > 

And colored console would also be a plus, and it would be needed into the IConsole interface

Maybe this is too much for now, buy I like adding wood to the fire :)

@adambajguz
Copy link
Owner

adambajguz commented Apr 6, 2021

  • cd can be achieved by copy-pasting the logic of ScopeDirective to a command EDIT: maybe creating some service for this is not a bad idea
  • I have never thought about custom prompt, but it's doable even as minor release: please create a new issue
  • IConsole has colors, however there is no ANSI colors support - you can also create an issue for this. Or maybe you think of colored input? E.g. [blue]book[\blue] [red]--help[\red]

Don't worry, I also like adding wood to fire ;)

@iskandersierra
Copy link
Author

I'll try to make a first draft of a design (just interfaces and data objects) this weekend to start discussing concrete issues, before starting to do any implementation. It shouldn't take me to communicate my ideas better to you, and it would allow me to know your thoughts about them.

Do you think this approach is suitable to you?

@adambajguz
Copy link
Owner

Yes, interfaces are crucial so it a great idea to start from them.

@adambajguz adambajguz pinned this issue Apr 8, 2021
@adambajguz adambajguz modified the milestones: v3.1, v3.2 Apr 11, 2021
@adambajguz
Copy link
Owner

adambajguz commented Jul 10, 2021

@iskandersierra Hi, I have started implementing dynamic commands on feature/issue-186_dynamic-commands. Currently, there is a very basic but partially working version without command providers, dynamic arguments binding, and any other fancy logic or huge refactor.

Some hints where to look first:

  • InteractiveModeExample.Commands.AddDynamicCommand
  • InteractiveModeExample.Commands.RemoveDynamicCommand
  • InteractiveModeExample.Commands.SampleDynamicCommand

@adambajguz adambajguz modified the milestones: v3.2, v4.0 Jul 11, 2021
@adambajguz adambajguz linked a pull request Jul 12, 2021 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Typin Typin library related issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants