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

Chaining AddCommand #78

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Chaining AddCommand #78

wants to merge 7 commits into from

Conversation

norrbacka
Copy link

@norrbacka norrbacka commented Oct 11, 2022

Maybe this is possible but I just could not figure out how. But the structure I have in my real program is this:

using Cocona;
using ping_cli;
var app = CoconaApp.CreateBuilder().Build();
app.AddCommand("set-config", Commands.SetConfig());
app.AddCommand("get-config", Commands.GetConfig());
app.AddCommand("ping", Commands.Ping());
app.AddCommand("settle", Commands.SettlePaymentOrder());
app.Run();

And I would like to have it chained: (because it looks good)

using Cocona;
using ping_cli;
CoconaApp
  .CreateBuilder()
  .Build()
  .AddCommand("set-config", Commands.SetConfig())
  .AddCommand("get-config", Commands.GetConfig())
  .AddCommand("ping", Commands.Ping())
  .AddCommand("settle", Commands.SettlePaymentOrder())
  .Run();

It would be pretty cool that have it like that. But it looked not that easy to change so I did an alternate version using a new functions name AddCommands that takes an array of commands, and I made an extension method to cast it to CoconaApp so the run function could be called.

Quite ugly, and I only wrote two tests.

Do not have the time right now to do something better, so maybe take this as an suggestion rather than something actually good.

@norrbacka
Copy link
Author

norrbacka commented Oct 11, 2022

Fair warning - this is a suggestion, and not really production worthy.

If you have any pointers on how this could be achieved, or would like to add it to the backlog, or decline it from happening - that is fine :)

@norrbacka
Copy link
Author

Okay, I got it working using an extension method:


    public static CoconaApp AddCommand(this CoconaApp app, string name, Delegate @delegate) 
    {
        _ = CommandsBuilderExtensions.AddCommand(app, name, @delegate);
        return app;
    }

So now this compiles and is confirming to work:

using Cocona;
using ping_cli;
CoconaApp
    .CreateBuilder()
    .Build()
    .AddCommand("set-config", Commands.SetConfig())
    .AddCommand("get-config", Commands.GetConfig())
    .AddCommand("ping", Commands.Ping())
    .AddCommand("settle", Commands.SettlePaymentOrder())
    .Run();

So maybe just ignore it, or add the extension method perhaps somewhere? Maybe it just me anyways, and it was quite ezy to write anyways

@norrbacka norrbacka marked this pull request as draft October 11, 2022 17:56
@mayuki
Copy link
Owner

mayuki commented Nov 4, 2022

Sorry for the late reply. Thank you for your proposal.👍

I did not introduce a chain (Fluent API) for AddCommand because it is based on ASP.NET Core's Minimal APIs.
For example, AddCommand returns a builder to customize a Command, but an API to add commands to it is a bit strange.

In ASP.NET Core, the example is as follows:

app.MapGet("/hello", () => "Hello!").AllowAnonymous().WithName("Hello");
app.MapGet("/bye", () => "Bye!").RequireAuthorization().WithName("Bye");
app.MapGet("/hello", () => "Hello!")
   .AllowAnonymous()
   .WithName("Hello")
   .MapGet("/bye", () => "Bye!")
   .RequireAuthorization()
   .WithName("Bye");

app.MapGet("/foo", () => "Foo").MapGet("/bar", () => "bar");

I feel this is a bit confusing with the mapping of endpoints and their configuration mixed in.

If Cocona had an API where AddCommand itself takes a delegate for configuration, the chain can be naturally applied.

app.AddCommand("set-config", () => { ... }, options => options.Alias("sc"))
   .AddCommand("get-config", () => { ... }, options => options.Alias("gc"));

I am also concerned that the API may look to allow sub-commands to be added to the command.
However, I think a chain API like the Fluent API is worth considering, so thanks again.

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