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

Add overload to allow one to set default command within the configuration delegate. #1469

Closed
AraHaan opened this issue Feb 18, 2024 · 10 comments
Labels

Comments

@AraHaan
Copy link

AraHaan commented Feb 18, 2024

Is your feature request related to a problem? Please describe.
For basic apps one can set the default command outside of the .Configure function.
That is alright until you want for:

  • An overload of .Configure that requires to pass in TSettings as a type parameter, which is then used for Action<IConfiguator<TSettings> to make use of the generic version of that interface, which can be used to configure additional things that is normally not possible without the generic version.

Describe the solution you'd like
For a change like this to be accepted, AraHaan@636ddb5

Describe alternatives you've considered
Perhaps having it as an extension method that extends CommandApp?

Additional context

// set the console title.
Console.Title = "Elskom workload cross-platform installer";

// Need to register the code pages provider for code that parses
// and later needs ISO-8859-2
Encoding.RegisterProvider(
    CodePagesEncodingProvider.Instance);

// Test that it loads
_ = Encoding.GetEncoding("ISO-8859-2");
var app = new CommandApp();
app.Configure(config =>
{
    _ = config.AddCommand<InstallCommand>("install").WithDescription("Installs the Workload.");
    _ = config.AddCommand<UninstallCommand>("uninstall").WithDescription("Uninstalls the Workload.");
    _ = config.AddCommand<UpdateCommand>("update").WithDescription("Updates the Workload.");
});

var finalArgs = new List<string>();
var firstArg = args.FirstOrDefault()?.Trim().ToLowerInvariant() ?? string.Empty;
if (firstArg is not "install" and not "uninstall" and not "update")
{
    finalArgs.Add("install");
}

if (args.Length != 0)
{
    finalArgs.AddRange(args);
}

using (NuGetHelper.HttpClient = new HttpClient())
{
    var result = await app.RunAsync(finalArgs).ConfigureAwait(false);
    Console.Title = "";
    return result;
}

This code above could be greatly simplified if I could configure using the overload version proposed here to set the default command instead of making a dummy list and possibly having bugs in my code, I then could also directly pass in args to app.RunAsync which will also improve performance of my application for free as well.

@AraHaan
Copy link
Author

AraHaan commented Feb 18, 2024

I guess another alternative would be adding a way to have AddCommand where one can specify it to be a default command (bool value with default of it being false) as well, with a name different from the default name for said default command. 😄

@patriksvensson
Copy link
Contributor

@AraHaan You can set the default command by using CommandApp<T>.

var app = new CommandApp<MyDefaultCommand>();

@AraHaan
Copy link
Author

AraHaan commented Feb 19, 2024

@AraHaan You can set the default command by using CommandApp<T>.

var app = new CommandApp<MyDefaultCommand>();

Could, but that does not allow one to specify a specific name for the default command either. So I am thinking of ways around that as well.

@patriksvensson
Copy link
Contributor

@AraHaan I'm sorry, but I don't quite follow what you mean.

@patriksvensson
Copy link
Contributor

This is how Spectre.Console default commands are designed to work. If you want to add a named command as well, you do it in the configure phase.

var app = new CommandApp<InstallCommand>(); // Default command
app.Configure(config =>
{
    _ = config.AddCommand<InstallCommand>("install").WithDescription("Installs the Workload.");
    _ = config.AddCommand<UninstallCommand>("uninstall").WithDescription("Uninstalls the Workload.");
    _ = config.AddCommand<UpdateCommand>("update").WithDescription("Updates the Workload.");
});

@FrankRay78
Copy link
Contributor

This is how Spectre.Console default commands are designed to work. If you want to add a named command as well, you do it in the configure phase.

Has this now cleared up your issue @AraHaan?

@FrankRay78 FrankRay78 added the area-CLI Command-Line Interface label Mar 7, 2024
@AraHaan
Copy link
Author

AraHaan commented Mar 11, 2024

Yes, but I would like a way to do it with and without the generic version of CommandApp which would be great (not to mention help unify both versions similarly which would help make it more maintainable as then the generic version could be defined as this exactly:

public class CommandApp<T> : CommandApp
{
   // ctor that bootstraps setting the passed in T as the default command.
}

@patriksvensson
Copy link
Contributor

patriksvensson commented Mar 11, 2024

You can do that now as well:

var app = new CommandApp();
app.SetDefaultCommand<InstallCommand>();
app.Configure(config =>
{
    _ = config.AddCommand<InstallCommand>("install").WithDescription("Installs the Workload.");
    _ = config.AddCommand<UninstallCommand>("uninstall").WithDescription("Uninstalls the Workload.");
    _ = config.AddCommand<UpdateCommand>("update").WithDescription("Updates the Workload.");
});

@FrankRay78
Copy link
Contributor

Hi @AraHaan, I'm just triaging the open CLI issues. Has Patrik's response fully addressed your ask?

@FrankRay78
Copy link
Contributor

Closing. Feel free to reopen if something remains outstanding.

@github-project-automation github-project-automation bot moved this from Todo 🕑 to Done 🚀 in Spectre Console Mar 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Done 🚀
Development

No branches or pull requests

3 participants