-
Notifications
You must be signed in to change notification settings - Fork 47
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
feat: Implement ViewMapNotFoundException
s
#2425
base: main
Are you sure you want to change the base?
Conversation
/// <summary> | ||
/// Represents an exception that is thrown when a view map is not found in the view registry. | ||
/// </summary> | ||
public class ViewMapNotFoundException : Exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @xperiandri, thanks for the PR! Maybe we could simplify a bit and have something like:
public enum ViewMapSearchType
View,
ViewModel,
Data,
ResultData
public class ViewMapNotFoundException : Exception
public ViewMapNotFoundException(ViewMapSearchType searchType, Type? type = null)
: base(CreateNotFoundMessage(searchType, type))
public ViewMapNotFoundException(ViewMapSearchType searchType, Type? type, Exception innerException)
:base(CreateNotFoundMessage(searchType, type), innerException)
private static string CreateNotFoundMessage(ViewMapSearchType searchType, Type? type)
Or if it's preferable to have multiple exception types (so they can be caught independently), we could define a base exception class like ViewMapNotFoundExceptionBase
. In this base class, we can define the CreateTypeNotFoundMessage
method such as:
CreateTypeNotFoundMessage(string typeName, Type? type)
=> $"The {typeName} type {type} was not found in the view registry.";
This method can then be used by the other exception classes that inherit from ViewMapNotFoundExceptionBase
. Remember to use the enum ViewMapSearchType
, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defined a base class.
Type comes from different errors, so different exception types needed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please update your fork with the latest changes from the main repo? It includes an update to the System.Text.Json
package to 8.0.4. The previous version 8.0.1 now has vulnerabilities.
Repo contributor can rebase my branch onto the latest dev with 2 clicks |
Why do we need a specific exception type? I dont see Try/Catch happening on that specific exception type being useful. Couldnt we just have a better error message? Ideally those discussions would happen at the "specification" level on a discussion first please so there's less back and forth. |
I will go that way next time |
So as I mentioned before, if you forget to register some mapping you will see ArgumentException sequence contains no elements. It means nothing to understand what was the issue |
GitHub Issue : closes #2424
PR Type
What kind of change does this PR introduce?
What is the current behavior?
Meaningless exception
What is the new behavior?
ViewMapNotFoundException
derived exceptionsPR Checklist
Please check if your PR fulfills the following requirements:
Screenshots Compare Test Run
results.