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 CommandExceptionHandler #1264

Closed

Conversation

JKamsker
Copy link

@JKamsker JKamsker commented Jul 24, 2023

Reason:
I need a way of handling exceptions while having the typeresolver around. (To log to an implementation of ILogger for e.g)
Unfortunately, the existing exceptionhandler does not provide a way of accessing the ITypeResolver

Usage:

public class GenericExceptionHandler : ICommandExceptionHandler
{
    // You can inject services here
    public GenericExceptionHandler()
    {
        
    }

        public bool Handle(CommandExceptionArgs args)
        {
            AnsiConsole.MarkupLine($"[red]{args.Exception.Message}[/]");
            return true;
        }
}

public class CommandGenericExceptionHandler : ICommandExceptionHandler<MyCommand>
{
    // You can inject services here
    public CommandGenericExceptionHandler()
    {
        
    }

        public bool Handle(CommandExceptionArgs args)
        {
            AnsiConsole.MarkupLine($"[red]{args.Exception.Message}[/]");
            return true;
        }
}

Then register them during startup:

//...
services.AddTransient<ICommandExceptionHandler, GenericExceptionHandler>();
services.AddTransient< ICommandExceptionHandler<MyCommand>, CommandGenericExceptionHandler>();

//...
var app = new CommandApp(new TypeRegistrar(services));

@nils-a
Copy link
Contributor

nils-a commented Sep 16, 2023

Wouldn't it be easier to add the current ITypeResolver to the existing ErrorHandling?

@JKamsker JKamsker force-pushed the CommandExceptionHandler branch 2 times, most recently from 3139825 to 96dd137 Compare September 25, 2023 08:07
@JKamsker
Copy link
Author

@nils-a Apologies, I'm having a bit of difficulty understanding what you're suggesting. Could you please explain it?

OT: Force pushed because i wanted a cleaner diff. The CommandExecutor appearantly is in mixed mode (LF & CRLF). vscode messes it up.

@nils-a
Copy link
Contributor

nils-a commented Sep 26, 2023

@JKamsker the currently existing way to handle exceptions is:

var app = new CommandApp<DefaultCommand>();
app.Configure(config =>
{
    config.SetExceptionHandler(err =>
    {
        AnsiConsole.WriteException(err);
        // or whatever....
    });
});

I don't quite like the idea of adding another, different way to do exception handling.

So: If you need the ITypeResolver while handling exceptions, my proposal was to add the ITypeResolver to the exception handler like this:

var app = new CommandApp<DefaultCommand>();
app.Configure(config =>
{
    config.SetExceptionHandler((err, resolver) =>
    {
        var console = (IAnsiConsole)resolver.Resolve(typeof(IAnsiConsole));
        console.WriteException(err);
        // or whatever....
    });
});

@JKamsker
Copy link
Author

@nils-a Oh yea, that seems like a neat idea! Why haven't i thought about this? 😅
Would adding an overload like SetExceptionHandler<TCommand> be acceptable?

@nils-a
Copy link
Contributor

nils-a commented Sep 26, 2023

Would adding an overload like SetExceptionHandler be acceptable?

Hm. Do you really need different handlers for different Commands? I have the feeling this would make things rather complicated.
Wouldn't it be easier, in that case, to create your own ICommand that is capable of handling exceptions?

FrankRay78 added a commit to FrankRay78/spectre.console that referenced this pull request Jan 6, 2024
FrankRay78 added a commit to FrankRay78/spectre.console that referenced this pull request Jan 6, 2024
FrankRay78 added a commit to nils-a/spectre.console that referenced this pull request Jan 6, 2024
@FrankRay78
Copy link
Contributor

Superseded by #1411.

@FrankRay78 FrankRay78 closed this Jan 6, 2024
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.

3 participants