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

Rename Exception<T> and have non-generic base #573

Closed
stephentoub opened this issue Apr 21, 2023 · 1 comment · Fixed by #701
Closed

Rename Exception<T> and have non-generic base #573

stephentoub opened this issue Apr 21, 2023 · 1 comment · Fixed by #701
Assignees

Comments

@stephentoub
Copy link
Member

stephentoub commented Apr 21, 2023

The base class for Semantic Kernel exceptions is Exception<TErrorCode>. This is too general a name, even though it's part of the Microsoft.SemanticKernel.Diagnostics namespace. Once a developer imports the namespace, catch blocks like catch (Exception<TErrorCode>) to catch all SK exceptions look much more broad and general-purpose than it is. At the same time, it's not broad enough to catch all SK exceptions, because it'll only catch those for that specific TErrorCode enum type.

Further, the TErrorCode needn't actually be part of the exception's identity. The base exception type doesn't store anything of that type, and instead just accepts an instance as a ctor parameter and uses it to build up a message. That can be done just accepting an object; if you're about to throw an exception, one additional allocation to box an enum value doesn't matter.

It'd be better to have just

public abstract class SemanticKernelException : Exception
{
    protected SemanticKernelException();
    protected SemanticKernelException(string? message);
    protected SemanticKernelException(string? message, Exception? innerException);
}

to address both concerns. Derived exception types can include an error code in whatever message they pass down. If we want a BuildMessage helper to assist with that, it can be an internal / private protected static method on the base exception type.

cc: @dluc, @shawncal

@dluc dluc self-assigned this Apr 22, 2023
@dluc
Copy link
Collaborator

dluc commented Apr 26, 2023

  1. rename for clarity
  2. remove T from base class, no impact

lemillermicrosoft pushed a commit that referenced this issue Apr 27, 2023
…701)

### Motivation and Context

Fixes #573

### Description

- Make the base class non-generic and rename it to SKException
- Remove unnecessary private ctors from derived types
- Clean up how messages are generated
dluc pushed a commit that referenced this issue Apr 29, 2023
…701)

### Motivation and Context

Fixes #573

### Description

- Make the base class non-generic and rename it to SKException
- Remove unnecessary private ctors from derived types
- Clean up how messages are generated
dehoward pushed a commit to lemillermicrosoft/semantic-kernel that referenced this issue Jun 1, 2023
…icrosoft#701)

### Motivation and Context

Fixes microsoft#573

### Description

- Make the base class non-generic and rename it to SKException
- Remove unnecessary private ctors from derived types
- Clean up how messages are generated
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 a pull request may close this issue.

2 participants